[GitHub] drill issue #600: DRILL-4373: Drill and Hive have incompatible timestamp rep...

[parthchandra]

Github user parthchandra commented on the issue:

https://github.com/apache/drill/pull/600
  
@vdiravka Looks like the test 
TestHiveStorage.readAllSupportedHiveDataTypesNativeParquet:214 is also failing. 
(The timestamp_field value is not matching the baseline). Can you take a look?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill issue #573: Drill 4858

[parthchandra]

Github user parthchandra commented on the issue:

https://github.com/apache/drill/pull/573
  
Oops. This change breaks a bunch of unit tests. 
-1 until we fix that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill issue #639: DRILL-4706: Fragment planning causes Drillbits to read rem...

[ppadma]

Github user ppadma commented on the issue:

https://github.com/apache/drill/pull/639
  
Updated the JIRA with details on how current algorithm works, why remote 
reads were happening and the new algorithm details.
https://issues.apache.org/jira/browse/DRILL-4706



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #640: Merge pull request #2 from apache/master

[gparai]

Github user gparai closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #640: Merge pull request #2 from apache/master

[gparai]

GitHub user gparai reopened a pull request:

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

Merge pull request #2 from apache/master

Sync with apache master

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/gparai/drill master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/640.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #640


commit fcde922184f87f23424c38f2d921a3261e97555a
Author: Gautam Parai 
Date:   2016-10-06T19:26:10Z

Merge pull request #1 from apache/master

Sync with apache master




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #640: Merge pull request #2 from apache/master

[gparai]

Github user gparai closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #640: Merge pull request #2 from apache/master

[gparai]

GitHub user gparai opened a pull request:

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

Merge pull request #2 from apache/master

Sync with apache master

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/gparai/drill master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/640.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #640


commit fcde922184f87f23424c38f2d921a3261e97555a
Author: Gautam Parai 
Date:   2016-10-06T19:26:10Z

Merge pull request #1 from apache/master

Sync with apache master




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: Limit the number of output parquet files in CTAS

[Andries Engelbrecht]

You can try and set store.partition.hash_distribute to true, but it is still 
listed as an alpha feature.

You can also add a sort operation (order by) to the CTAS statement to force a 
single data stream at output. I believe this was discussed a while back on the 
user list.

Ideally you want to look at the data set size and how much parallelism would 
work best in your environment for reading the data later.

--Andries


> On Oct 31, 2016, at 12:57 PM, François Méthot  wrote:
> 
> Hi,
> 
> Is there a way to limit the number of files produced by a CTAS query ?
> I would like the speed benefits of having hundreds of scanner fragment but
> don't want to deal with hundreds of output files.
> 
> Our usecase right now is using 880 thread to scan and produce a report
> output spread over... 880 parquets files.
> Each resulting file is ~7M.
> 
> Only way I found to reduce those files to smaller set is  to a perform
> second CTAS query on the aggregated files with planner.width.max_per_query
> set to smaller number.
> 
> Any possible way to do this in one query?
> 
> Thanks
> Francois

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85803203
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -603,33 +596,16 @@ public ListHoldingResultsListener(UserProtos.RunQuery 
query) {
 
 @Override
 public void submissionFailed(UserException ex) {
-  // or  !client.isActive()
-  if (ex.getCause() instanceof ChannelClosedException) {
-if (reconnect()) {
--- End diff --

does it mean there won't be any reconnection attempt?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85800423
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
+return SASL_NOMECH;
+}
+
+bool isSupportedByServer = false;
+for (size_t i = 0; i < mechanisms.size(); i++) {
+std::string mechanism = mechanisms[i];
+if (authMechanismToUse.compare(mechanism) == 0) {
+isSupportedByServer = true;
+}
+}
+if (!isSupportedByServer) {
+return SASL_NOMECH;
+}
+boost::algorithm::to_lower(authMechanismToUse);
+chosenMech = authMechanismToUse;
+std::string sasMechanismToUse = NULL;
+if (authMechanismToUse.compare("plain") == 0) {
--- End diff --

you can just use == or != to compare two strings in C++


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled 

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85823003
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -328,44 +304,73 @@ protected void consumeHandshake(ChannelHandlerContext 
ctx, UserToBitHandshake in
   public BitToUserHandshake getHandshakeResponse(UserToBitHandshake 
inbound) throws Exception {
 logger.trace("Handling handshake from user to bit. {}", inbound);
 
-
 // if timeout is unsupported or is set to false, disable timeout.
 if (!inbound.hasSupportTimeout() || !inbound.getSupportTimeout()) {
   connection.disableReadTimeout();
   logger.warn("Timeout Disabled as client doesn't support it.", 
connection.getName());
 }
 
-BitToUserHandshake.Builder respBuilder = 
BitToUserHandshake.newBuilder()
+final BitToUserHandshake.Builder respBuilder = 
BitToUserHandshake.newBuilder()
 .setRpcVersion(UserRpcConfig.RPC_VERSION);
 
 try {
-  if (inbound.getRpcVersion() != UserRpcConfig.RPC_VERSION) {
-final String errMsg = String.format("Invalid rpc version. 
Expected %d, actual %d.",
-UserRpcConfig.RPC_VERSION, inbound.getRpcVersion());
+  if (!SUPPORTED_RPC_VERSIONS.contains(inbound.getRpcVersion())) {
+final String errMsg = String.format("Invalid rpc version. 
Expected %s, actual %d.",
+SUPPORTED_RPC_VERSIONS, inbound.getRpcVersion());
 
 return handleFailure(respBuilder, 
HandshakeStatus.RPC_VERSION_MISMATCH, errMsg, null);
   }
 
-  if (authenticator != null) {
+  connection.setHandshake(inbound);
+
+  if (authFactory == null) { // authentication is disabled
+
connection.finalizeSession(inbound.getCredentials().getUserName());
+respBuilder.setStatus(HandshakeStatus.SUCCESS);
+return respBuilder.build();
+  }
+
+  if (inbound.getRpcVersion() == NON_SASL_RPC_VERSION_SUPPORTED) { 
// for backward compatibility
+final String userName = inbound.getCredentials().getUserName();
+if (logger.isTraceEnabled()) {
+  logger.trace("User {} on connection {} is using an older 
client (Drill version <= 1.8).",
+  userName, connection.getRemoteAddress());
+}
 try {
   String password = "";
   final UserProperties props = inbound.getProperties();
   for (int i = 0; i < props.getPropertiesCount(); i++) {
 Property prop = props.getProperties(i);
-if (UserSession.PASSWORD.equalsIgnoreCase(prop.getKey())) {
+if 
(ConnectionParameters.PASSWORD.equalsIgnoreCase(prop.getKey())) {
   password = prop.getValue();
   break;
 }
   }
-  
authenticator.authenticate(inbound.getCredentials().getUserName(), password);
+  final PlainMechanism plainMechanism = 
authFactory.getPlainMechanism();
+  if (plainMechanism == null) {
+throw new UserAuthenticationException("The server no 
longer supports username/password" +
+" based authentication. Please talk to your system 
administrator.");
+  }
+  plainMechanism.getAuthenticator()
+  .authenticate(userName, password);
+  connection.changeHandlerTo(handler);
+  connection.finalizeSession(userName);
+  respBuilder.setStatus(HandshakeStatus.SUCCESS);
--- End diff --

should the rpc version of the response be changed to 
`NON_SASL_RPC_VERSION_SUPPORTED` in that specific case?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85800113
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
--- End diff --

tricky/incorrect... llvm compiler doesn't allow it for example


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85798971
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
+return SASL_NOMECH;
+}
+
+bool isSupportedByServer = false;
+for (size_t i = 0; i < mechanisms.size(); i++) {
--- End diff --

you can use the find function...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85806353
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/plain/PlainServer.java
 ---
@@ -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.
+ */
+package org.apache.drill.exec.rpc.security.plain;
+
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+import javax.security.sasl.SaslServerFactory;
+import java.io.IOException;
+import java.security.Provider;
+import java.util.Map;
+
+/**
+ * Plain SaslServer implementation. See https://tools.ietf.org/html/rfc4616
+ */
+public class PlainServer implements SaslServer {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(PlainServer.class);
+
+  public static class PlainServerFactory implements SaslServerFactory {
+
+@Override
+public SaslServer createSaslServer(final String mechanism, final 
String protocol, final String serverName,
+   final Map props, final 
CallbackHandler cbh)
+throws SaslException {
+  return "PLAIN".equals(mechanism) ?
+  props == null || 
"false".equals(props.get(Sasl.POLICY_NOPLAINTEXT)) ?
+  new PlainServer(cbh) :
+  null :
+  null;
+}
+
+@Override
+public String[] getMechanismNames(final Map props) {
+  return props == null || 
"false".equals(props.get(Sasl.POLICY_NOPLAINTEXT)) ?
+  new String[]{"PLAIN"} :
+  new String[0];
+}
+  }
+
+  @SuppressWarnings("serial")
+  public static class PlainServerProvider extends Provider {
+
+public PlainServerProvider() {
+  super("PlainServer", 1.0, "PLAIN SASL Server Provider");
+  put("SaslServerFactory.PLAIN", PlainServerFactory.class.getName());
+}
+  }
+
+  private CallbackHandler cbh;
+  private boolean completed = false;
+  private String authorizationID;
+
+  PlainServer(final CallbackHandler cbh) throws SaslException {
+if (cbh == null) {
+  throw new SaslException("PLAIN: A callback handler must be 
specified");
+}
+this.cbh = cbh;
+  }
+
+  @Override
+  public String getMechanismName() {
+return "PLAIN";
+  }
+
+  @Override
+  public byte[] evaluateResponse(byte[] response) throws SaslException {
+if (completed) {
+  throw new IllegalStateException("PLAIN authentication already 
completed");
+}
+
+if (response == null) {
+  throw new SaslException("Received null response");
+}
+
+final String payload;
+try {
+  payload = new String(response, "UTF-8");
+} catch (final Exception e) {
--- End diff --

use `new String(response, java.nio.charset.StandardChaset.UTF_8)`: the 
behaviour of that call is well-defined (and doesn't throw exception)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802232
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
--- End diff --

you can use a smart pointer to guard this object and free it automatically


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85800898
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.hpp ---
@@ -564,6 +570,34 @@ class ZookeeperImpl{
 std::string m_rootDir;
 };
 
+class SaslAuthenticatorImpl {
--- End diff --

I suggest to move it to a separate unit, as the class is fairly complex, 
and this is an helper class.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85819216
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -78,21 +101,241 @@ public void submitQuery(UserResultsListener 
resultsListener, RunQuery query) {
 send(queryResultHandler.getWrappedListener(resultsListener), 
RpcType.RUN_QUERY, query, QueryId.class);
   }
 
-  public void connect(RpcConnectionHandler handler, 
DrillbitEndpoint endpoint,
-  UserProperties props, UserBitShared.UserCredentials 
credentials) {
+  public CheckedFuture connect(DrillbitEndpoint 
endpoint, ConnectionParameters parameters,
+   UserCredentials 
credentials) {
+final FutureHandler handler = new FutureHandler();
 UserToBitHandshake.Builder hsBuilder = UserToBitHandshake.newBuilder()
 .setRpcVersion(UserRpcConfig.RPC_VERSION)
 .setSupportListening(true)
 .setSupportComplexTypes(supportComplexTypes)
 .setSupportTimeout(true)
-.setCredentials(credentials);
+.setCredentials(credentials)
+.setProperties(parameters.serializeForServer());
+this.parameters = parameters;
+
+
connectAsClient(queryResultHandler.getWrappedConnectionHandler(handler),
+hsBuilder.build(), endpoint.getAddress(), endpoint.getUserPort());
+return handler;
+  }
+
+  /**
+   * Check (after {@link #connect connecting}) if server requires 
authentication.
+   *
+   * @return true if server requires authentication
+   */
+  public boolean serverRequiresAuthentication() {
+return supportedAuthMechs != null;
+  }
+
+  /**
+   * Returns a list of supported authentication mechanism. If called 
before {@link #connect connecting},
+   * returns null. If called after {@link #connect connecting}, returns a 
list of supported mechanisms
+   * iff authentication is required.
+   *
+   * @return list of supported authentication mechanisms
+   */
+  public List getSupportedAuthenticationMechanisms() {
+return supportedAuthMechs;
+  }
+
+  /**
+   * Authenticate to the server asynchronously. Returns a future that 
{@link CheckedFuture#checkedGet results}
+   * in null if authentication succeeds, or throws a {@link SaslException} 
with relevant message if
+   * authentication fails.
+   *
+   * This method uses parameters provided at {@link #connect connection 
time} and override them with the
+   * given parameters, if any.
+   *
+   * @param overrides parameter overrides
+   * @return result of authentication request
+   */
+  public CheckedFuture authenticate(final 
ConnectionParameters overrides) {
+if (supportedAuthMechs == null) {
+  throw new IllegalStateException("Server does not require 
authentication.");
+}
+parameters.merge(overrides);
+
+final SettableFuture settableFuture = SettableFuture.create(); 
// future used in SASL exchange
+final CheckedFuture future =
+new AbstractCheckedFuture(settableFuture) {
+
+  @Override
+  protected SaslException mapException(Exception e) {
+if (connection != null) {
+  connection.close(); // to ensure connection is dropped
+}
+if (e instanceof ExecutionException) {
+  final Throwable cause = e.getCause();
+  if (cause instanceof SaslException) {
+return new SaslException("Authentication failed: " + 
cause.getMessage(), cause);
+  }
+}
+return new SaslException("Authentication failed 
unexpectedly.", e);
+  }
+};
 
-if (props != null) {
-  hsBuilder.setProperties(props);
+final ClientAuthenticationProvider authenticationProvider;
+try {
+  authenticationProvider =
+  
UserAuthenticationUtil.getClientAuthenticationProvider(parameters, 
supportedAuthMechs);
+} catch (final SaslException e) {
+  settableFuture.setException(e);
+  return future;
 }
 
-
this.connectAsClient(queryResultHandler.getWrappedConnectionHandler(handler),
-hsBuilder.build(), endpoint.getAddress(), endpoint.getUserPort());
+final String providerName = authenticationProvider.name();
+logger.trace("Will try to login for {} mechanism.", providerName);
+final UserGroupInformation ugi;
+try {
+  ugi = authenticationProvider.login(parameters);
+} catch (final SaslException e) {
+  

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85819718
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -299,6 +260,20 @@ public ChannelFuture getChannelClosureFuture() {
 public SocketAddress getRemoteAddress() {
   return getChannel().remoteAddress();
 }
+
+@Override
+public void close() {
+  super.close();
--- End diff --

if the super method throws, saslServer won't be cleaned. Is that okay?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85803810
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/AuthenticationMechanismFactory.java
 ---
@@ -0,0 +1,169 @@
+/**
+ * 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.security;
+
+import com.google.common.base.Function;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Sets;
+import org.apache.drill.common.AutoCloseables;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.map.CaseInsensitiveMap;
+import org.apache.drill.common.scanner.persistence.ScanResult;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.DrillbitStartupException;
+import org.apache.drill.exec.rpc.security.plain.PlainMechanism;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticator;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticatorFactory;
+import org.apache.drill.exec.security.LoginManager;
+
+import javax.annotation.Nullable;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public class AuthenticationMechanismFactory implements AutoCloseable {
+  private static final org.slf4j.Logger logger =
+  
org.slf4j.LoggerFactory.getLogger(AuthenticationMechanismFactory.class);
+
+  public static final String AUTHENTICATION_MECHANISMS = 
"drill.exec.security.auth.mechanisms";
+
+  // Mapping: SASL name -> mechanism
+  // See AuthenticationMechanism#getMechanismName
--- End diff --

this method doesn't exist!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85819136
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -78,21 +101,241 @@ public void submitQuery(UserResultsListener 
resultsListener, RunQuery query) {
 send(queryResultHandler.getWrappedListener(resultsListener), 
RpcType.RUN_QUERY, query, QueryId.class);
   }
 
-  public void connect(RpcConnectionHandler handler, 
DrillbitEndpoint endpoint,
-  UserProperties props, UserBitShared.UserCredentials 
credentials) {
+  public CheckedFuture connect(DrillbitEndpoint 
endpoint, ConnectionParameters parameters,
+   UserCredentials 
credentials) {
+final FutureHandler handler = new FutureHandler();
 UserToBitHandshake.Builder hsBuilder = UserToBitHandshake.newBuilder()
 .setRpcVersion(UserRpcConfig.RPC_VERSION)
 .setSupportListening(true)
 .setSupportComplexTypes(supportComplexTypes)
 .setSupportTimeout(true)
-.setCredentials(credentials);
+.setCredentials(credentials)
+.setProperties(parameters.serializeForServer());
+this.parameters = parameters;
+
+
connectAsClient(queryResultHandler.getWrappedConnectionHandler(handler),
+hsBuilder.build(), endpoint.getAddress(), endpoint.getUserPort());
+return handler;
+  }
+
+  /**
+   * Check (after {@link #connect connecting}) if server requires 
authentication.
+   *
+   * @return true if server requires authentication
+   */
+  public boolean serverRequiresAuthentication() {
+return supportedAuthMechs != null;
+  }
+
+  /**
+   * Returns a list of supported authentication mechanism. If called 
before {@link #connect connecting},
+   * returns null. If called after {@link #connect connecting}, returns a 
list of supported mechanisms
+   * iff authentication is required.
+   *
+   * @return list of supported authentication mechanisms
+   */
+  public List getSupportedAuthenticationMechanisms() {
+return supportedAuthMechs;
+  }
+
+  /**
+   * Authenticate to the server asynchronously. Returns a future that 
{@link CheckedFuture#checkedGet results}
+   * in null if authentication succeeds, or throws a {@link SaslException} 
with relevant message if
+   * authentication fails.
+   *
+   * This method uses parameters provided at {@link #connect connection 
time} and override them with the
+   * given parameters, if any.
+   *
+   * @param overrides parameter overrides
+   * @return result of authentication request
+   */
+  public CheckedFuture authenticate(final 
ConnectionParameters overrides) {
+if (supportedAuthMechs == null) {
+  throw new IllegalStateException("Server does not require 
authentication.");
+}
+parameters.merge(overrides);
+
+final SettableFuture settableFuture = SettableFuture.create(); 
// future used in SASL exchange
+final CheckedFuture future =
+new AbstractCheckedFuture(settableFuture) {
+
+  @Override
+  protected SaslException mapException(Exception e) {
+if (connection != null) {
+  connection.close(); // to ensure connection is dropped
+}
+if (e instanceof ExecutionException) {
+  final Throwable cause = e.getCause();
+  if (cause instanceof SaslException) {
+return new SaslException("Authentication failed: " + 
cause.getMessage(), cause);
+  }
+}
+return new SaslException("Authentication failed 
unexpectedly.", e);
+  }
+};
 
-if (props != null) {
-  hsBuilder.setProperties(props);
+final ClientAuthenticationProvider authenticationProvider;
+try {
+  authenticationProvider =
+  
UserAuthenticationUtil.getClientAuthenticationProvider(parameters, 
supportedAuthMechs);
+} catch (final SaslException e) {
+  settableFuture.setException(e);
+  return future;
 }
 
-
this.connectAsClient(queryResultHandler.getWrappedConnectionHandler(handler),
-hsBuilder.build(), endpoint.getAddress(), endpoint.getUserPort());
+final String providerName = authenticationProvider.name();
+logger.trace("Will try to login for {} mechanism.", providerName);
+final UserGroupInformation ugi;
+try {
+  ugi = authenticationProvider.login(parameters);
+} catch (final SaslException e) {
+  

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802137
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
--- End diff --

since this object was created by the SASL library, should sasl_dispose be 
called instead?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85819519
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -246,28 +160,75 @@ protected void handle(UserClientConnectionImpl 
connection, int rpcType, ByteBuf
   public class UserClientConnectionImpl extends RemoteConnection 
implements UserClientConnection {
 
 private UserSession session;
+private SaslServer saslServer;
+private RequestHandler currentHandler;
--- End diff --

this field can be final I guess


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85800948
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.hpp ---
@@ -564,6 +570,34 @@ class ZookeeperImpl{
 std::string m_rootDir;
 };
 
+class SaslAuthenticatorImpl {
+
+public:
+
+SaslAuthenticatorImpl(const DrillUserProperties* const properties);
+
+~SaslAuthenticatorImpl();
+
+int init(std::vector mechanisms, std::string 
,
--- End diff --

use const reference as much as possible.

The API also mix up C and C++ types, maybe this class should only expose an 
API based on C++ types, and manage the conversion from/to C types as a better 
encapsulation model


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85804286
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/AuthenticationMechanismFactory.java
 ---
@@ -0,0 +1,169 @@
+/**
+ * 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.security;
+
+import com.google.common.base.Function;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Sets;
+import org.apache.drill.common.AutoCloseables;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.map.CaseInsensitiveMap;
+import org.apache.drill.common.scanner.persistence.ScanResult;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.DrillbitStartupException;
+import org.apache.drill.exec.rpc.security.plain.PlainMechanism;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticator;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticatorFactory;
+import org.apache.drill.exec.security.LoginManager;
+
+import javax.annotation.Nullable;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public class AuthenticationMechanismFactory implements AutoCloseable {
+  private static final org.slf4j.Logger logger =
+  
org.slf4j.LoggerFactory.getLogger(AuthenticationMechanismFactory.class);
+
+  public static final String AUTHENTICATION_MECHANISMS = 
"drill.exec.security.auth.mechanisms";
+
+  // Mapping: SASL name -> mechanism
+  // See AuthenticationMechanism#getMechanismName
+  private final Map mechanisms = 
CaseInsensitiveMap.newHashMapWithExpectedSize(5);
+
+  @SuppressWarnings("unchecked")
+  public AuthenticationMechanismFactory(final DrillConfig config, final 
ScanResult scan,
+final LoginManager loginManager) 
throws DrillbitStartupException {
+if (!config.hasPath(ExecConstants.AUTHENTICATION_MECHANISMS)) {
+  return;
+}
+
+final List configuredMechanisms = 
config.getStringList(ExecConstants.AUTHENTICATION_MECHANISMS);
+logger.debug("Configuring authentication mechanisms: {}", 
configuredMechanisms);
+// transform all names to uppercase
+final Set configuredMechanismsSet = 
Sets.newHashSet(Iterators.transform(configuredMechanisms.iterator(),
+new Function() {
+  @Nullable
+  @Override
+  public String apply(@Nullable String input) {
+return input == null ? null : input.toUpperCase();
+  }
+}));
+
+// PLAIN mechanism need special handling due to UserAuthenticator
+if (configuredMechanismsSet.contains(PlainMechanism.MECHANISM_NAME)) {
+  // instantiated here, but closed in PlainMechanism#close
+  final UserAuthenticator userAuthenticator = 
UserAuthenticatorFactory.createAuthenticator(config, scan);
+  final PlainMechanism mechanism = new 
PlainMechanism(userAuthenticator);
+  mechanisms.put(PlainMechanism.MECHANISM_NAME, mechanism);
+  configuredMechanismsSet.remove(PlainMechanism.MECHANISM_NAME);
--- End diff --

you can do it in the if clause:
`if (configuredMechanismSet.remove(PlainMechanism.MECHANISM_NAME))`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85798339
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
+return SASL_NOMECH;
+}
+
+bool isSupportedByServer = false;
+for (size_t i = 0; i < mechanisms.size(); i++) {
+std::string mechanism = mechanisms[i];
+if (authMechanismToUse.compare(mechanism) == 0) {
+isSupportedByServer = true;
+}
+}
+if (!isSupportedByServer) {
+return SASL_NOMECH;
+}
+boost::algorithm::to_lower(authMechanismToUse);
+chosenMech = authMechanismToUse;
+std::string sasMechanismToUse = NULL;
+if (authMechanismToUse.compare("plain") == 0) {
+sasMechanismToUse = "plain";
+} else if (authMechanismToUse.compare("kerberos") == 0) {
+sasMechanismToUse = "gssapi";
+} else {
+return SASL_NOMECH;
+}
+
+// create
+

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802665
  
--- Diff: contrib/native/client/src/include/drill/common.hpp ---
@@ -34,7 +34,7 @@
 #include 
 #include 
 
-#define DRILL_RPC_VERSION 5
+#define DRILL_RPC_VERSION 6
--- End diff --

does it mean the client won't work with an older server version?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85799157
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
--- End diff --

like previously authMechanismToUse is not a pointer. You can remove the = 
NULL part.

I actually tried on Mac, and seems like the C++ lib doesn't like 
std::string(NULL) (it segfaults)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802847
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -224,19 +222,15 @@ public synchronized void connect(String connect, 
Properties props) throws RpcExc
   }
 
   final ArrayList endpoints = new 
ArrayList<>(clusterCoordinator.getAvailableEndpoints());
-  checkState(!endpoints.isEmpty(), "No DrillbitEndpoint can be found");
+  if (endpoints.isEmpty()) {
--- End diff --

why stop using checkState()?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85804609
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/AuthenticationMechanismFactory.java
 ---
@@ -0,0 +1,169 @@
+/**
+ * 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.security;
+
+import com.google.common.base.Function;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Sets;
+import org.apache.drill.common.AutoCloseables;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.map.CaseInsensitiveMap;
+import org.apache.drill.common.scanner.persistence.ScanResult;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.DrillbitStartupException;
+import org.apache.drill.exec.rpc.security.plain.PlainMechanism;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticator;
+import org.apache.drill.exec.rpc.user.security.UserAuthenticatorFactory;
+import org.apache.drill.exec.security.LoginManager;
+
+import javax.annotation.Nullable;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public class AuthenticationMechanismFactory implements AutoCloseable {
+  private static final org.slf4j.Logger logger =
+  
org.slf4j.LoggerFactory.getLogger(AuthenticationMechanismFactory.class);
+
+  public static final String AUTHENTICATION_MECHANISMS = 
"drill.exec.security.auth.mechanisms";
+
+  // Mapping: SASL name -> mechanism
+  // See AuthenticationMechanism#getMechanismName
+  private final Map mechanisms = 
CaseInsensitiveMap.newHashMapWithExpectedSize(5);
+
+  @SuppressWarnings("unchecked")
+  public AuthenticationMechanismFactory(final DrillConfig config, final 
ScanResult scan,
+final LoginManager loginManager) 
throws DrillbitStartupException {
+if (!config.hasPath(ExecConstants.AUTHENTICATION_MECHANISMS)) {
+  return;
+}
+
+final List configuredMechanisms = 
config.getStringList(ExecConstants.AUTHENTICATION_MECHANISMS);
+logger.debug("Configuring authentication mechanisms: {}", 
configuredMechanisms);
+// transform all names to uppercase
+final Set configuredMechanismsSet = 
Sets.newHashSet(Iterators.transform(configuredMechanisms.iterator(),
+new Function() {
+  @Nullable
+  @Override
+  public String apply(@Nullable String input) {
+return input == null ? null : input.toUpperCase();
+  }
+}));
+
+// PLAIN mechanism need special handling due to UserAuthenticator
+if (configuredMechanismsSet.contains(PlainMechanism.MECHANISM_NAME)) {
+  // instantiated here, but closed in PlainMechanism#close
+  final UserAuthenticator userAuthenticator = 
UserAuthenticatorFactory.createAuthenticator(config, scan);
+  final PlainMechanism mechanism = new 
PlainMechanism(userAuthenticator);
+  mechanisms.put(PlainMechanism.MECHANISM_NAME, mechanism);
+  configuredMechanismsSet.remove(PlainMechanism.MECHANISM_NAME);
+  logger.trace("Plain mechanism enabled.");
+}
+
+// Then, load other mechanisms, if any
+if (!configuredMechanismsSet.isEmpty()) {
+  final Collection 
mechanismImpls =
+  scan.getImplementations(AuthenticationMechanism.class);
+  logger.debug("Found AuthenticationMechanism implementations: {}", 
mechanismImpls);
+
+  for (Class clazz : 
mechanismImpls) {
+final SaslMechanism annotation = 
clazz.getAnnotation(SaslMechanism.class);
+if (annotation == null) {
+  logger.warn("{} 

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802018
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
--- End diff --

you can guard if you store the sasl connection into a smart pointer (where 
you specify sasl_dispose as the destructor)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85798863
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
+return SASL_NOMECH;
+}
+
+bool isSupportedByServer = false;
+for (size_t i = 0; i < mechanisms.size(); i++) {
+std::string mechanism = mechanisms[i];
--- End diff --

you can use a const reference


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85797710
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
--- End diff --

do you have to make it part of SaslAuthenticatorImpl interface? (maybe it 
could be a simple function in an anonymous namespace)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85802469
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
+std::string ,
+const char **out,
+unsigned *outlen) {
+// set params
+std::string authMechanismToUse = NULL;
+for (size_t i = 0; i < m_properties->size(); i++) {
+const std::map::const_iterator it =
+
DrillUserProperties::USER_PROPERTIES.find(m_properties->keyAt(i));
+if (it == DrillUserProperties::USER_PROPERTIES.end()) {
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_USERNAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting name" << 
std::endl;)
+m_username = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_PASSWORD)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting password" << 
std::endl;)
+m_password = m_properties->valueAt(i);
+m_secret = (sasl_secret_t *) malloc(sizeof(sasl_secret_t) + 
m_password.length());
+authMechanismToUse = "plain";
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_AUTH_MECHANISM)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+authMechanismToUse = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_NAME)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service name" << 
std::endl;)
+m_servicename = m_properties->valueAt(i);
+continue;
+}
+if (IS_BITSET((*it).second, USERPROP_FLAGS_SERVICE_HOST)) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Setting service host" << 
std::endl;)
+m_servicehost = m_properties->valueAt(i);
+}
+}
+if (authMechanismToUse == NULL) {
+return SASL_NOMECH;
+}
+
+bool isSupportedByServer = false;
+for (size_t i = 0; i < mechanisms.size(); i++) {
+std::string mechanism = mechanisms[i];
+if (authMechanismToUse.compare(mechanism) == 0) {
+isSupportedByServer = true;
+}
+}
+if (!isSupportedByServer) {
+return SASL_NOMECH;
+}
+boost::algorithm::to_lower(authMechanismToUse);
+chosenMech = authMechanismToUse;
+std::string sasMechanismToUse = NULL;
+if (authMechanismToUse.compare("plain") == 0) {
--- End diff --

"plain"/"gssapi" should probably be represented by constants


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85798619
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
+*psecret = authenticator->m_secret;
+}
+   return SASL_OK;
+}
+
+SaslAuthenticatorImpl::SaslAuthenticatorImpl(const DrillUserProperties* 
const properties) :
+m_properties(properties), m_pConnection(NULL), m_secret(NULL), 
m_servicename(NULL), m_servicehost(NULL) {
+}
+
+SaslAuthenticatorImpl::~SaslAuthenticatorImpl() {
+if (m_secret) {
+free(m_secret);
+}
+// may be to use negotiated security layers before disposing in the 
future
+if (m_pConnection) {
+sasl_dispose(_pConnection);
+}
+m_pConnection = NULL;
+}
+
+int SaslAuthenticatorImpl::init(std::vector mechanisms,
--- End diff --

use a const ref for mechanism


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85798045
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
+
+if ((SASL_CB_USER == id || SASL_CB_AUTHNAME == id)
+&& username != NULL) {
+*result = username->c_str();
+//*len = (unsigned int) username->length();
+}
+return SASL_OK;
+}
+
+static int SaslAuthenticatorImpl::passwordCallback(sasl_conn_t *conn, void 
*context, int id, sasl_secret_t **psecret) {
+const SaslAuthenticatorImpl* const authenticator = (const 
SaslAuthenticatorImpl* const) context;
+
+if (SASL_CB_PASS == id) {
+const std::string password = authenticator->m_password;
+const size_t length = password.length();
+authenticator->m_secret->len = length;
+std::memcpy(authenticator->m_secret->data, password.c_str(), 
length);
--- End diff --

isn't the context outliving the callback? (meaning we would not need like 
to copy the string, like for usernameCallback?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85797062
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -427,6 +511,121 @@ connectionStatus_t 
DrillClientImpl::validateHandshake(DrillUserProperties* prope
 getMessage(ERR_CONN_AUTHFAIL,
 this->m_handshakeErrorId.c_str(),
 this->m_handshakeErrorMsg.c_str()));
+case exec::user::AUTH_REQUIRED: {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Server requires SASL 
authentication." << std::endl;)
+SaslAuthenticatorImpl saslAuthenticator(properties);
+int saslResult = 0;
+std::string chosenMech;
+const char *out;
+unsigned outlen;
+saslResult = saslAuthenticator.init(m_mechanisms, 
chosenMech, , );
+if (saslResult != SASL_OK) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Authenticator 
init failed. Code: " << saslResult << std::endl;)
+return handleConnError(CONN_AUTH_FAILED, "User 
authentication init failed.");
+}
+if (NULL == out) {
+out = 
(&::google::protobuf::internal::kEmptyString)->c_str();
+}
+// send initial response
+{
+exec::user::SaslMessage response;
+response.set_data(out, outlen);
+response.set_mechanism(chosenMech[0]);
+
response.set_status(exec::user::SaslStatus::SASL_START);
+{
+boost::lock_guard 
lock(this->m_dcMutex);
+int32_t coordId = this->getNextCoordinationId();
+
+OutBoundRpcMessage out_msg(exec::rpc::REQUEST, 
exec::user::SASL_MESSAGE, coordId, );
+sendSync(out_msg);
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Sent SASL 
init response, id: " << coordId
+  << " result: " 
<< saslResult << std::endl;)
+}
+}
+
+bool done = false;
+while (saslResult == SASL_OK || saslResult == 
SASL_CONTINUE) {
+if (done) {
+break;
+}
+// receive challenge
+InBoundRpcMessage inboundMessage;
+readMessage(inboundMessage);
+if (m_pError) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Something 
failed." << std::endl;)
+return CONN_AUTH_FAILED;
+}
+exec::user::SaslMessage challenge;
+
challenge.ParseFromArray(inboundMessage.m_pbody.data(), 
inboundMessage.m_pbody.size());
--- End diff --

you should check the return value for parsing errors


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85801077
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.hpp ---
@@ -564,6 +570,34 @@ class ZookeeperImpl{
 std::string m_rootDir;
 };
 
+class SaslAuthenticatorImpl {
+
+public:
+
+SaslAuthenticatorImpl(const DrillUserProperties* const properties);
+
+~SaslAuthenticatorImpl();
+
+int init(std::vector mechanisms, std::string 
,
+ const char **out, unsigned *outlen);
+
+int step(const char* const in, const unsigned inlen, const char 
**out, unsigned *outlen) const;
+
+static int passwordCallback(sasl_conn_t *conn, void *context, int 
id, sasl_secret_t **psecret);
--- End diff --

you don't need to expose the callbacks I believe


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85792599
  
--- Diff: common/src/main/java/org/apache/drill/common/KerberosUtil.java ---
@@ -0,0 +1,40 @@
+/**
+ * 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.common;
+
+public final class KerberosUtil {
+//  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(KerberosUtil.class);
+
+  public static final String KERBEROS_SASL_NAME = "GSSAPI";
+
+  public static final String KERBEROS_SIMPLE_NAME = "KERBEROS";
+
+  // primary/instance@REALM
+  public static String getPrincipalFromParts(final String primary, final 
String instance, final String realm) {
--- End diff --

what if instance is null (seems to be allowed to only have primary@REALM)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85795087
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -248,7 +267,7 @@ connectionStatus_t DrillClientImpl::recvHandshake(){
 }
 
 m_io_service.reset();
-if (DrillClientConfig::getHandshakeTimeout() > 0){
+if (false){
--- End diff --

is it for testing purposes?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85646140
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/security/SimpleServer.java
 ---
@@ -0,0 +1,138 @@
+/**
+ * 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.security;
+
+import com.google.common.primitives.Ints;
+
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+import javax.security.sasl.SaslServerFactory;
+import java.util.Map;
+
+public class SimpleServer implements SaslServer {
+
+  private boolean completed;
+  private String authorizationId;
+  private final int total;
+  private int count = 0;
+
+  SimpleServer(final int total) {
+this.total = total;
+  }
+
+  @Override
+  public String getMechanismName() {
+return SimpleProvider.MECHANISM_NAME;
+  }
+
+  @Override
+  public byte[] evaluateResponse(byte[] response) throws SaslException {
+if (completed) {
+  throw new IllegalStateException("SimpleSasl authentication already 
completed");
+}
+if (response == null || response.length < 1) {
+  throw new SaslException("Received challenge is empty when secret 
expected");
+}
+
+if (count == 0) { // first expect authorization ID
+  //This SaslServer simply permits a client to authenticate according 
to whatever username
+  //was supplied in client's response[]
+  authorizationId = new String(response);
--- End diff --

it's probably risky to rely on the default encoding to convert the byte 
array into a string


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645751
  
--- Diff: contrib/native/client/cmakeModules/FindSASL.cmake ---
@@ -0,0 +1,55 @@
+#
+# 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.
+#
+
+# - Try to find Cyrus SASL
+
+if (MSVC)
+if(${CMAKE_BUILD_TYPE} MATCHES "Debug")
+set(SASL_BuildOutputDir "Debug")
+else()
+set(SASL_BuildOutputDir "Release")
+endif()
+if("${SASL_HOME}_" MATCHES  "^_$")
+message(" ")
+message("- Please set the cache variable SASL_HOME to point to the 
directory with the Cyrus SASL source.")
+message("- CMAKE will look for Cyrus SASL include files in 
$SASL_HOME/src/c/include.")
--- End diff --

those are the instructions for zookeeper, but are they the same for Cyrus 
SASL? A quick look at the source don't show any src/c/include directory.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85792190
  
--- Diff: common/src/main/java/org/apache/drill/common/KerberosUtil.java ---
@@ -0,0 +1,40 @@
+/**
+ * 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.common;
+
+public final class KerberosUtil {
+//  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(KerberosUtil.class);
+
+  public static final String KERBEROS_SASL_NAME = "GSSAPI";
+
+  public static final String KERBEROS_SIMPLE_NAME = "KERBEROS";
+
+  // primary/instance@REALM
+  public static String getPrincipalFromParts(final String primary, final 
String instance, final String realm) {
+return primary + "/" + instance + "@" + realm;
+  }
+
+  // primary/instance@REALM
+  public static String[] splitPrincipalIntoParts(final String principal) {
--- End diff --

what about malformed principal strings? or it seems that instance might be 
optional too.
You might want to add unit tests to cover multiple cases


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645840
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -1849,4 +2048,150 @@ void ZookeeperImpl:: debugPrint(){
 }
 }
 
+typedef int (*sasl_callback_proc_t)(void); // see sasl_callback_ft
+
+static int SaslAuthenticatorImpl::userNameCallback(void *context, int id, 
const char **result, unsigned *len) {
+const std::string* const username = (const std::string* const) context;
--- End diff --

avoid C-style cast


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645966
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -591,7 +584,7 @@ public void runQuery(QueryType type, String plan, 
UserResultsListener resultsLis
 client.submitQuery(resultsListener, 
newBuilder().setResultsMode(STREAM_FULL).setType(type).setPlan(plan).build());
   }
 
-  private class ListHoldingResultsListener implements UserResultsListener {
+  protected class ListHoldingResultsListener implements 
UserResultsListener {
--- End diff --

why the change of visibility?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85794422
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -102,18 +106,33 @@ connectionStatus_t DrillClientImpl::connect(const 
char* connStr){
 return handleConnError(CONN_ZOOKEEPER_ERROR, 
getMessage(ERR_CONN_ZOOKEEPER, zook.getError().c_str()));
 }
 zook.close();
-m_bIsDirectConnection=true;  
+m_bIsDirectConnection=true;
 }else if(!strcmp(protocol.c_str(), "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;  
+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 servicename = NULL;
--- End diff --

servicename is a value, not a pointer. If you don't want to set a value, 
simply remove the assignment part (otherwise, it's like calling 
std::string(NULL))


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645935
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -88,21 +84,22 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Strings;
-import com.google.common.util.concurrent.AbstractCheckedFuture;
+
 import com.google.common.util.concurrent.SettableFuture;
 
+import javax.security.sasl.SaslException;
+
 /**
  * Thin wrapper around a UserClient that handles connect/close and 
transforms
  * String into ByteBuf.
  */
 public class DrillClient implements Closeable, ConnectionThrottle {
   private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(DrillClient.class);
 
-  private static final ObjectMapper objectMapper = new ObjectMapper();
+  protected static final ObjectMapper objectMapper = new ObjectMapper();
--- End diff --

why this change? I was not able to find a change in the diff which requires 
the change of visibility


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645696
  
--- Diff: contrib/native/client/cmakeModules/FindSASL.cmake ---
@@ -0,0 +1,55 @@
+#
+# 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.
+#
+
+# - Try to find Cyrus SASL
+
+if (MSVC)
+if(${CMAKE_BUILD_TYPE} MATCHES "Debug")
+set(SASL_BuildOutputDir "Debug")
+else()
+set(SASL_BuildOutputDir "Release")
+endif()
+if("${SASL_HOME}_" MATCHES  "^_$")
+message(" ")
+message("- Please set the cache variable SASL_HOME to point to the 
directory with the Cyrus SASL source.")
+message("- CMAKE will look for Cyrus SASL include files in 
$SASL_HOME/src/c/include.")
+message("- CMAKE will look for Cyrus SASL library files in 
$SASL_HOME/src/c/Debug or $SASL_HOME/src/c/Release.")
+else()
+FILE(TO_CMAKE_PATH ${SASL_HOME} SASL_HomePath)
+set(SASL_LIB_PATHS ${SASL_HomePath}/src/c/${SASL_BuildOutputDir} 
${SASL_HomePath}/src/c/x64/${SASL_BuildOutputDir} )
+
+find_path(SASL_INCLUDE_DIR sasl.h 
${Zookeeper_HomePath}/src/c/include)
--- End diff --

Zookeeper reference


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85796563
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -427,6 +511,121 @@ connectionStatus_t 
DrillClientImpl::validateHandshake(DrillUserProperties* prope
 getMessage(ERR_CONN_AUTHFAIL,
 this->m_handshakeErrorId.c_str(),
 this->m_handshakeErrorMsg.c_str()));
+case exec::user::AUTH_REQUIRED: {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Server requires SASL 
authentication." << std::endl;)
+SaslAuthenticatorImpl saslAuthenticator(properties);
+int saslResult = 0;
+std::string chosenMech;
+const char *out;
+unsigned outlen;
+saslResult = saslAuthenticator.init(m_mechanisms, 
chosenMech, , );
+if (saslResult != SASL_OK) {
+DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Authenticator 
init failed. Code: " << saslResult << std::endl;)
+return handleConnError(CONN_AUTH_FAILED, "User 
authentication init failed.");
+}
+if (NULL == out) {
+out = 
(&::google::protobuf::internal::kEmptyString)->c_str();
--- End diff --

we should not use protobuf internal constants. Either use the empty string 
"" here (it might be okay if some of the functions you call out with are not 
keeping reference on it after returning), or create a special constant.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85796780
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -427,6 +511,121 @@ connectionStatus_t 
DrillClientImpl::validateHandshake(DrillUserProperties* prope
 getMessage(ERR_CONN_AUTHFAIL,
 this->m_handshakeErrorId.c_str(),
 this->m_handshakeErrorMsg.c_str()));
+case exec::user::AUTH_REQUIRED: {
--- End diff --

this case block is too big I think, you might want to extract it (or part 
of it) into methods or helper functions


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #578: DRILL-4280: Kerberos Authentication

[laurentgo]

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

https://github.com/apache/drill/pull/578#discussion_r85645955
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -285,9 +279,14 @@ public synchronized boolean reconnect() {
   }
 
   private void connect(DrillbitEndpoint endpoint) throws RpcException {
-final FutureHandler f = new FutureHandler();
-client.connect(f, endpoint, props, getUserCredentials());
-f.checkedGet();
+client.connect(endpoint, parameters, 
getUserCredentials()).checkedGet();
+if (client.serverRequiresAuthentication()) {
--- End diff --

what if the server doesn't require authentication, shouldn't the client 
wait too?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill issue #573: Drill 4858

[parthchandra]

Github user parthchandra commented on the issue:

https://github.com/apache/drill/pull/573
  
+1 for the fix. I'll add the unit test based on the examples in the JIRA.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Limit the number of output parquet files in CTAS

[François Méthot]

Hi,

Is there a way to limit the number of files produced by a CTAS query ?
I would like the speed benefits of having hundreds of scanner fragment but
don't want to deal with hundreds of output files.

Our usecase right now is using 880 thread to scan and produce a report
output spread over... 880 parquets files.
Each resulting file is ~7M.

Only way I found to reduce those files to smaller set is  to a perform
second CTAS query on the aggregated files with planner.width.max_per_query
set to smaller number.

Any possible way to do this in one query?

Thanks
Francois

[GitHub] drill issue #635: DRILL-4927 (part 2): Add support for Null Equality Joins (...

[KulykRoman]

Github user KulykRoman commented on the issue:

https://github.com/apache/drill/pull/635
  
Added tests with t1.key = t2.key AND ((t1.data=t2.data) OR (t1.data IS NULL 
AND t2.data IS NULL)) pattern.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: Jason's operator test framework?

[Jason Altekruse]

Hey Paul,

I included basic tests for a good portion of the operators with the
framework itself. You can check out this class [1] for the examples. Feel
free to send along any questions.

A known limitation:
- There is no way to currently declare assertions about where the outgoing
batch boundaries are expected to occur, it currently concatenates all of
the outgoing batches together before comparing them to a single result set.
This includes dereferencing selection vectors that are produced by the
filter operator (selection vector 2, which is a bitmask over a single batch
to represent valid records that matched the filter) and the sort operator
(selection vector 4, which is a pointer sort reordering over many batches
that has not yet been rewritten)

[1] -
https://github.com/apache/drill/blob/master/exec/java-exec/src/test/java/org/apache/drill/exec/physical/unit/BasicPhysicalOpUnitTest.java

Jason Altekruse
Software Engineer at Dremio
Apache Drill Committer

On Mon, Oct 31, 2016 at 9:50 AM, Paul Rogers  wrote:

> Hi Jason & All,
>
> A couple months back Jason presented some very nice work where he was able
> to create a test framework for individual operators.
>
> Jason, is your framework documented anywhere? Or, can you point me to some
> tests that use the framework?
>
> Thanks!
>
> - Paul

RE: [HANGOUT] Topics for 11/1/16

[Dave Oshinsky]

Hi Paul,
Can we talk a bit about working to improve decimal type support?  My related PR 
is waiting for review:
https://issues.apache.org/jira/browse/DRILL-4834

Thanks,
Dave Oshinsky

-Original Message-
From: Paul Rogers [mailto:prog...@maprtech.com] 
Sent: Monday, October 31, 2016 12:33 PM
To: dev@drill.apache.org; u...@drill.apache.org
Subject: [HANGOUT] Topics for 11/1/16

Hi All,

Our bi-weekly hangout is tomorrow (11/01/16, 10 AM PT). Please respond with 
suggested topics. We will also ask for additional topics at the beginning of 
the hangout.

See you then,

- Paul


***Legal Disclaimer***
"This communication may contain confidential and privileged material for the
sole use of the intended recipient. Any unauthorized review, use or distribution
by others is strictly prohibited. If you have received the message by mistake,
please advise the sender by reply email and delete the message. Thank you."
**

Jason's operator test framework?

[Paul Rogers]

Hi Jason & All,

A couple months back Jason presented some very nice work where he was able to 
create a test framework for individual operators.

Jason, is your framework documented anywhere? Or, can you point me to some 
tests that use the framework?

Thanks!

- Paul

[HANGOUT] Topics for 11/1/16

[Paul Rogers]

Hi All,

Our bi-weekly hangout is tomorrow (11/01/16, 10 AM PT). Please respond with 
suggested topics. We will also ask for additional topics at the beginning of 
the hangout.

See you then,

- Paul

[GitHub] drill pull request #639: New fragment placement algorithm based on locality ...

[ppadma]

GitHub user ppadma opened a pull request:

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

New fragment placement algorithm based on locality of data.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ppadma/drill DRILL-4706

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/639.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #639


commit 5a7fd87d7cfc26b03389c593bd20349e82dd484c
Author: Padma Penumarthy 
Date:   2016-10-07T14:38:09Z

New fragment placement algorithm based on locality of data.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: Time for a 1.9 Release?

[Subbu Srinivasan]

+1.

On Sun, Oct 30, 2016 at 10:23 PM, Paul Rogers  wrote:

> For release numbers, 1.10 (then 1.11, 1.12, …) seems like a good idea.
>
> At first it may seem odd to go to 1.10 from 1.9. Might people get confused
> between 1.10 and 1.1.0? But, there is precedence. Tomcat’s latest 7-series
> release is 7.0.72. Java is on 8u112. And so on.
>
> I like the idea of moving to 2.0 later when the team introduces a major
> change, rather than by default just because the numbers roll around. For
> example, Hadoop when to 2.x when YARN was introduced. Impala appears to
> have moved to 2.0 when they added Spill to disk for some (all?) operators.
>
> - Paul
>
> > On Oct 28, 2016, at 10:34 AM, Sudheesh Katkam 
> wrote:
> >
> > Hi Drillers,
> >
> > We have a reasonable number of fixes and features since the last release
> > [1]. Releasing itself takes a while; so I propose we start the 1.9
> release
> > process.
> >
> > I volunteer as the release manager, unless there are objections.
> >
> > We should also discuss what the release version number should be after
> 1.9.
> >
> > Thank you,
> > Sudheesh
> >
> > [1] https://issues.apache.org/jira/browse/DRILL/fixforversion/12337861
>
>

[jira] [Created] (DRILL-4984) Limit 0 raises NullPointerException on JDBC storage sources

[Holger Kiel (JIRA)]

Holger Kiel created DRILL-4984:
--

 Summary: Limit 0 raises NullPointerException on JDBC storage 
sources
 Key: DRILL-4984
 URL: https://issues.apache.org/jira/browse/DRILL-4984
 Project: Apache Drill
  Issue Type: Bug
  Components: Query Planning & Optimization
Affects Versions: 1.8.0, 1.9.0
 Environment: Latest 1.9 Snapshot, also 1.8 release version,
mysql-connector-java-5.1.30, mysql-connector-java-5.1.40
Reporter: Holger Kiel


NullPointerExceptions occur when a query with 'limit 0' is executed on a jdbc 
storage source (e.g. Mysql):

{code}
0: jdbc:drill:zk=local> select * from mysql.sugarcrm.sales_person limit 0;
Error: SYSTEM ERROR: NullPointerException


[Error Id: 6cd676fc-6db9-40b3-81d5-c2db044aeb77 on localhost:31010]

  (org.apache.drill.exec.work.foreman.ForemanException) Unexpected exception 
during fragment initialization: null
org.apache.drill.exec.work.foreman.Foreman.run():281
java.util.concurrent.ThreadPoolExecutor.runWorker():1142
java.util.concurrent.ThreadPoolExecutor$Worker.run():617
java.lang.Thread.run():745
  Caused By (java.lang.NullPointerException) null

org.apache.drill.exec.planner.sql.handlers.FindHardDistributionScans.visit():55
org.apache.calcite.rel.core.TableScan.accept():166
org.apache.calcite.rel.RelShuttleImpl.visitChild():53
org.apache.calcite.rel.RelShuttleImpl.visitChildren():68
org.apache.calcite.rel.RelShuttleImpl.visit():126
org.apache.calcite.rel.AbstractRelNode.accept():256
org.apache.calcite.rel.RelShuttleImpl.visitChild():53
org.apache.calcite.rel.RelShuttleImpl.visitChildren():68
org.apache.calcite.rel.RelShuttleImpl.visit():126
org.apache.calcite.rel.AbstractRelNode.accept():256

org.apache.drill.exec.planner.sql.handlers.FindHardDistributionScans.canForceSingleMode():45

org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.convertToDrel():262

org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.convertToDrel():290
org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.getPlan():168
org.apache.drill.exec.planner.sql.DrillSqlWorker.getPhysicalPlan():123
org.apache.drill.exec.planner.sql.DrillSqlWorker.getPlan():97
org.apache.drill.exec.work.foreman.Foreman.runSQL():1008
org.apache.drill.exec.work.foreman.Foreman.run():264
java.util.concurrent.ThreadPoolExecutor.runWorker():1142
java.util.concurrent.ThreadPoolExecutor$Worker.run():617
java.lang.Thread.run():745 (state=,code=0)
0: jdbc:drill:zk=local> select * from mysql.sugarcrm.sales_person limit 1;
+-+-+++-+
| id  | first_name  |   last_name| full_name  | manager_id  |
+-+-+++-+
| 1   | null| Administrator  | admin  | 0   |
+-+-+++-+
1 row selected (0,235 seconds)
{code}

Other datasources are okay:
{code}
0: jdbc:drill:zk=local> SELECT * FROM cp.`employee.json` LIMIT 0;
+--+---+---+-+--++-++--+-+---++-++-++--+-+-+--+
| fqn  | filename  | filepath  | suffix  | employee_id  | full_name  | 
first_name  | last_name  | position_id  | position_title  | store_id  | 
department_id  | birth_date  | hire_date  | salary  | supervisor_id  | 
education_level  | marital_status  | gender  | management_role  |
+--+---+---+-+--++-++--+-+---++-++-++--+-+-+--+
+--+---+---+-+--++-++--+-+---++-++-++--+-+-+--+
No rows selected (0,309 seconds)
{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-4983) Random failure while reading footer of hive generated parquet file

[Rahul Challapalli (JIRA)]

Rahul Challapalli created DRILL-4983:


 Summary: Random failure while reading footer of hive generated 
parquet file
 Key: DRILL-4983
 URL: https://issues.apache.org/jira/browse/DRILL-4983
 Project: Apache Drill
  Issue Type: Bug
Reporter: Rahul Challapalli
 Attachments: error.log

The below query seems to have failed while reading the footer of the first 
parquet file. There is also a metadata cache present which was generated with 
Drill 1.2.

{code}
select distinct dt from (select date_col dt from  
dfs.`/drill/testdata/parquet_date/metadata_cache/metadata_cache1.2_autogen/fewtypes_null_large`
  where date_col is not null
union all 
select c dt from dfs.`/drill/testdata/parquet_date/spark_generated/d4` where c 
is not null
union all
select l_shipdate dt from 
dfs.`/drill/testdata/parquet_date/dates_nodrillversion/drillgen2_lineitem`
union all
select l_shipdate dt from dfs.`/drill/testdata/parquet_date/fixeddate_lineitem`
) data order by dt asc limit 1000;
{code}

I attached the error from the logs and the dataset as well.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[GitHub] drill pull request #638: DRILL-4982: Separate Hive reader classes for differ...

[chunhui-shi]

GitHub user chunhui-shi opened a pull request:

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

DRILL-4982: Separate Hive reader classes for different data formats t…

…o improve performance.

1, Separating Hive reader classes allows optimization to apply on different 
classes in optimized ways. This  separation effectively avoid the performance 
degradation of scan.

2, Do not apply Skip footer/header mechanism on most Hive formats. This 
skip mechanism introduces extra checks on each incoming records.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/chunhui-shi/drill DRILL-4982

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/638.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #638


commit 1e240a0dfea7970798bd550204388fc0e9bc3d42
Author: chunhui-shi 
Date:   2016-10-30T08:29:06Z

DRILL-4982: Separate Hive reader classes for different data formats to 
improve performance.

1, Separating Hive reader classes allows optimization to apply on different 
classes in optimized ways. This  separation effectively avoid the performance 
degradation of scan.

2, Do not apply Skip footer/header mechanism on most Hive formats. This 
skip mechanism introduces extra checks on each incoming records.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---