Repository: drill Updated Branches: refs/heads/master 779703421 -> f1d1945b3
DRILL-5881:Java Client: [Threat Modeling] Drillbit may be spoofed by an attacker and this may lead to data being written to the attacker's target instead of Drillbit Also set connection to null after its closed This closes #999 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/40d09919 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/40d09919 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/40d09919 Branch: refs/heads/master Commit: 40d0991998a4174cf5d4b12f695380e97aba4fd3 Parents: 7797034 Author: Sorabh Hamirwasia <shamirwa...@maprtech.com> Authored: Mon Oct 16 14:58:31 2017 -0700 Committer: Parth Chandra <par...@apache.org> Committed: Thu Oct 19 17:12:38 2017 -0700 ---------------------------------------------------------------------- .../apache/drill/exec/rpc/user/UserClient.java | 69 +++- .../security/TestUserBitSaslCompatibility.java | 317 +++++++++++++++++++ .../org/apache/drill/exec/rpc/BasicClient.java | 1 + 3 files changed, 377 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/40d09919/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java index 99614bd..fb78812 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java @@ -202,7 +202,8 @@ public class UserClient .checkedGet(sslConfig.getHandshakeTimeout(), TimeUnit.MILLISECONDS); } catch (TimeoutException e) { String msg = new StringBuilder().append( - "Connecting to the server timed out. This is sometimes due to a mismatch in the SSL configuration between client and server. [ Exception: ") + "Connecting to the server timed out. This is sometimes due to a mismatch in the SSL configuration between" + + " client and server. [ Exception: ") .append(e.getMessage()).append("]").toString(); throw new NonTransientRpcException(msg); } @@ -210,15 +211,8 @@ public class UserClient connect(hsBuilder.build(), endpoint).checkedGet(); } - // Check if client needs encryption and server is not configured for encryption. - final boolean clientNeedsEncryption = properties.containsKey(DrillProperties.SASL_ENCRYPT) && Boolean - .parseBoolean(properties.getProperty(DrillProperties.SASL_ENCRYPT)); - - if (clientNeedsEncryption && !connection.isEncryptionEnabled()) { - throw new NonTransientRpcException( - "Client needs encrypted connection but server is not configured for " - + "encryption. Please check connection parameter or contact your administrator"); - } + // Validate if both client and server are compatible in their security requirements for the connection + validateSaslCompatibility(properties); if (serverAuthMechanisms != null) { try { @@ -229,6 +223,61 @@ public class UserClient } } + /** + * Validate that security requirements from client and Drillbit side is compatible. For example: It verifies if one + * side needs authentication / encryption then other side is also configured to support that security properties. + * @param properties - DrillClient connection parameters + * @throws NonTransientRpcException - When DrillClient security requirements doesn't match Drillbit side of security + * configurations. + */ + private void validateSaslCompatibility(DrillProperties properties) throws NonTransientRpcException { + + final boolean clientNeedsEncryption = properties.containsKey(DrillProperties.SASL_ENCRYPT) + && Boolean.parseBoolean(properties.getProperty(DrillProperties.SASL_ENCRYPT)); + + // Check if client needs encryption and server is not configured for encryption. + if (clientNeedsEncryption && !connection.isEncryptionEnabled()) { + throw new NonTransientRpcException( + "Client needs encrypted connection but server is not configured for encryption." + + " Please contact your administrator. [Warn: It may be due to wrong config or a security attack in" + + " progress.]"); + } + + // Check if client needs encryption and server doesn't support any security mechanisms. + if (clientNeedsEncryption && serverAuthMechanisms == null) { + throw new NonTransientRpcException( + "Client needs encrypted connection but server doesn't support any security mechanisms." + + " Please contact your administrator. [Warn: It may be due to wrong config or a security attack in" + + " progress.]"); + } + + // Check if client needs authentication and server doesn't support any security mechanisms. + if (clientNeedsAuth(properties) && serverAuthMechanisms == null) { + throw new NonTransientRpcException( + "Client needs authentication but server doesn't support any security mechanisms." + + " Please contact your administrator. [Warn: It may be due to wrong config or a security attack in" + + " progress.]"); + } + } + + /** + * Determine if client needs authenticated connection or not. It checks for following as an indication of + * requiring authentication from client side: + * 1) Any auth mechanism is provided in connection properties with DrillProperties.AUTH_MECHANISM parameter. + * 2) A service principal is provided in connection properties in which case we treat AUTH_MECHANISM as Kerberos + * 3) Username and Password is provided in connection properties in which case we treat AUTH_MECHANISM as Plain + * @param props - DrillClient connection parameters + * @return - true - If any of above 3 checks is successful. + * - false - If all the above 3 checks failed. + */ + private boolean clientNeedsAuth(DrillProperties props) { + + return !Strings.isNullOrEmpty(props.getProperty(DrillProperties.AUTH_MECHANISM)) || + !Strings.isNullOrEmpty(props.getProperty(DrillProperties.SERVICE_PRINCIPAL)) || + (props.containsKey(DrillProperties.USER) && !Strings.isNullOrEmpty(props.getProperty(DrillProperties.PASSWORD))); + + } + private CheckedFuture<Void, RpcException> connect(final UserToBitHandshake handshake, final DrillbitEndpoint endpoint) { final SettableFuture<Void> connectionSettable = SettableFuture.create(); http://git-wip-us.apache.org/repos/asf/drill/blob/40d09919/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSaslCompatibility.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSaslCompatibility.java b/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSaslCompatibility.java new file mode 100644 index 0000000..bbb957d --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSaslCompatibility.java @@ -0,0 +1,317 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.rpc.user.security; + + +import com.google.common.collect.Lists; +import com.typesafe.config.ConfigValueFactory; +import org.apache.drill.BaseTestQuery; +import org.apache.drill.common.config.DrillConfig; +import org.apache.drill.common.config.DrillProperties; +import org.apache.drill.exec.ExecConstants; +import org.apache.drill.exec.rpc.NonTransientRpcException; +import org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl; +import org.junit.BeforeClass; +import org.junit.Test; + +import javax.security.sasl.SaslException; +import java.util.Properties; + +import static junit.framework.TestCase.assertTrue; +import static junit.framework.TestCase.fail; + +/** + * Helps to test different scenarios based on security configuration on client and Drillbit side with respect to SASL + * and specifically using PLAIN mechanism + */ +public class TestUserBitSaslCompatibility extends BaseTestQuery { + //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestUserClient.class); + + @BeforeClass + public static void setup() { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + + updateTestCluster(1, newConfig, connectionProps); + } + + /** + * Test showing failure before SASL handshake when Drillbit is not configured for authentication whereas client + * explicitly requested for authentication. + * @throws Exception + */ + @Test + public void testDisableDrillbitAuth_EnableClientAuth() throws Exception { + + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(!(ex.getCause().getCause() instanceof SaslException)); + } + } + + /** + * Test showing failure before SASL handshake when Drillbit is not configured for authentication whereas client + * explicitly requested for encrypted connection. + * @throws Exception + */ + @Test + public void testDisableDrillbitAuth_EnableClientEncryption() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + connectionProps.setProperty(DrillProperties.SASL_ENCRYPT, "true"); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(!(ex.getCause().getCause() instanceof SaslException)); + } + } + + /** + * Test showing failure before SASL handshake when Drillbit is not configured for encryption whereas client explicitly + * requested for encrypted connection. + * @throws Exception + */ + @Test + public void testDisableDrillbitEncryption_EnableClientEncryption() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + connectionProps.setProperty(DrillProperties.SASL_ENCRYPT, "true"); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(!(ex.getCause().getCause() instanceof SaslException)); + } + } + + /** + * Test showing failure in SASL handshake when Drillbit is configured for authentication only whereas client doesn't + * provide any security properties like username/password in this case. + * @throws Exception + */ + @Test + public void testEnableDrillbitAuth_DisableClientAuth() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(ex.getCause().getCause() instanceof SaslException); + } + } + + /** + * Test showing failure in SASL handshake when Drillbit is configured for encryption whereas client doesn't provide any + * security properties like username/password in this case. + * @throws Exception + */ + @Test + public void testEnableDrillbitEncryption_DisableClientAuth() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(true)), + false); + + final Properties connectionProps = new Properties(); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(ex.getCause().getCause() instanceof SaslException); + } + } + + /** + * Test showing successful SASL handshake when both Drillbit and client side authentication is enabled using PLAIN + * mechanism. + * @throws Exception + */ + @Test + public void testEnableDrillbitClientAuth() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + + try { + updateTestCluster(1, newConfig, connectionProps); + } catch (Exception ex) { + fail(); + } + } + + /** + * Below test shows the failure in Sasl layer with client and Drillbit side encryption enabled using PLAIN + * mechanism. This is expected since PLAIN mechanism doesn't support encryption using SASL. Whereas same test + * setup using Kerberos or any other mechanism with encryption support will result in successful SASL handshake. + * @throws Exception + */ + @Test + public void testEnableDrillbitClientEncryption_UsingPlain() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, + ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)) + .withValue(ExecConstants.AUTHENTICATION_MECHANISMS, + ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))) + .withValue(ExecConstants.USER_ENCRYPTION_SASL_ENABLED, + ConfigValueFactory.fromAnyRef(true)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + connectionProps.setProperty(DrillProperties.PASSWORD, "anything works!"); + connectionProps.setProperty(DrillProperties.SASL_ENCRYPT, "true"); + + try { + updateTestCluster(1, newConfig, connectionProps); + fail(); + } catch (Exception ex) { + assertTrue(ex.getCause() instanceof NonTransientRpcException); + assertTrue(ex.getCause().getCause() instanceof SaslException); + } + } + + /** + * Test showing successful handshake when authentication is disabled on Drillbit side and client also + * doesn't provide any security properties in connection URL. + * @throws Exception + */ + @Test + public void testDisableDrillbitClientAuth() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(false)), + false); + + final Properties connectionProps = new Properties(); + + try { + updateTestCluster(1, newConfig, connectionProps); + } catch (Exception ex) { + fail(); + } + } + + /** + * Test showing successful handshake when authentication is disabled but impersonation is enabled on Drillbit side + * and client only provides USERNAME as a security property in connection URL. + * @throws Exception + */ + @Test + public void testEnableDrillbitImpersonation_DisableClientAuth() throws Exception { + final DrillConfig newConfig = new DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()) + .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, + ConfigValueFactory.fromAnyRef(false)) + .withValue(ExecConstants.IMPERSONATION_ENABLED, + ConfigValueFactory.fromAnyRef(true)) + .withValue(ExecConstants.IMPERSONATION_MAX_CHAINED_USER_HOPS, + ConfigValueFactory.fromAnyRef(3)), + false); + + final Properties connectionProps = new Properties(); + connectionProps.setProperty(DrillProperties.USER, "anonymous"); + + try { + updateTestCluster(1, newConfig, connectionProps); + } catch (Exception ex) { + fail(); + } + } + +} + http://git-wip-us.apache.org/repos/asf/drill/blob/40d09919/exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java ---------------------------------------------------------------------- diff --git a/exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java b/exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java index 0d80df6..0f4ef1b 100644 --- a/exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java +++ b/exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java @@ -254,6 +254,7 @@ public abstract class BasicClient<T extends EnumLite, CC extends ClientConnectio if (connection != null) { connection.close(); + connection = null; } } }
