You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/12 12:09:36 UTC
[GitHub] [drill] agozhiy opened a new pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
agozhiy opened a new pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022
…edentials using MapR Web Security Manager
# [DRILL-7637](https://issues.apache.org/jira/browse/DRILL-7637): Add an option to retrieve MapR SSL truststore/keystore credentials using MapR Web Security Manager
## Description
An option added to provide MapR truststore/keystore credentials using MapR Web Security Manager instead of setting passwords openly in the config.
## Documentation
To use this feature user need to:
- Install Drill on MapR platform.
- Enable the corresponding option in the Drill config:
`drill.exec.ssl.useMapRSSLConfig: true`
- Pass this option with the connection string using sqlline:
`./bin/sqlline -u "jdbc:drill:drillbit=node1.cluster.com:31010;enableTLS=true;useMapRSSLConfig=true"`
## Testing
- Unit tests passed w/ and w/o mapr profile.
- Functional/Advanced tests passed.
- Manually tested Web UI and sqlline with the feature enabled/disabled.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on issue #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
Posted by GitBox <gi...@apache.org>.
agozhiy commented on issue #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022#issuecomment-598684769
@ihuzenko, I addressed your comments, please review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] agozhiy merged pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
Posted by GitBox <gi...@apache.org>.
agozhiy merged pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022#discussion_r392142888
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLCredentialsProvider.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Provides an implementation of credentials provider depending on "useMapRSSLConfig" option value
+ * and whether Drill was built with mapr profile or not.
+ */
+abstract class SSLCredentialsProvider {
+
+ private static final String MAPR_CREDENTIALS_PROVIDER_CLIENT = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprClient";
+ private static final String MAPR_CREDENTIALS_PROVIDER_SERVER = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprServer";
+
+ /**
+ * Provides a concrete implementation of {@link SSLCredentialsProvider}.
+ *
+ * @param getPropertyMethod a reference to a method to retrieve credentials from config:
+ * <ul>
+ * <li>String parameter1 - property name</li>
+ * <li>String parameter2 - default value</li>
+ * <li>returns the property value or default value</li>
+ * </ul>
+ * @param mode CLIENT or SERVER
+ * @param useMapRSSLConfig use a MapR credential provider
+ * @return concrete implementation of SSLCredentialsProvider
+ */
+ static SSLCredentialsProvider getSSLCredentialsProvider(BiFunction<String, String, String> getPropertyMethod, SSLConfig.Mode mode, boolean useMapRSSLConfig) {
+ return useMapRSSLConfig ?
+ Optional.ofNullable(getMaprCredentialsProvider(mode))
+ .orElse(new SSLCredentialsProviderImpl(getPropertyMethod)) :
+ new SSLCredentialsProviderImpl(getPropertyMethod);
+ }
+
+ private static SSLCredentialsProvider getMaprCredentialsProvider(SSLConfig.Mode mode) {
+ try {
+ return (SSLCredentialsProvider) Class.forName(getMaprCredentialsProviderClassName(mode)).newInstance();
+ } catch (ClassNotFoundException | InstantiationException | IllegalAccessException e) {
+ // This is expected if built without MapR profile
+ return null;
+ }
+ }
+
+ private static String getMaprCredentialsProviderClassName(SSLConfig.Mode mode) {
+ switch (mode) {
+ case SERVER:
+ return MAPR_CREDENTIALS_PROVIDER_SERVER;
+ case CLIENT:
+ return MAPR_CREDENTIALS_PROVIDER_CLIENT;
+ }
+ return "";
+ }
+
+ abstract String getTrustStoreType(String propertyName, String defaultValue);
+
+ abstract String getTrustStoreLocation(String propertyName, String defaultValue);
+
+ abstract String getTrustStorePassword(String propertyName, String defaultValue);
+
+ abstract String getKeyStoreType(String propertyName, String defaultValue);
+
+ abstract String getKeyStoreLocation(String propertyName, String defaultValue);
+
+ abstract String getKeyStorePassword(String propertyName, String defaultValue);
+
+ abstract String getKeyPassword(String propertyName, String defaultValue);
+
+
+ /**
+ * Default implementation of {@link SSLCredentialsProvider}.
+ * Delegates retrieving credentials to a class where it is used.
+ */
+ private static class SSLCredentialsProviderImpl extends SSLCredentialsProvider {
+
+ private BiFunction<String, String, String> getPropertyMethod;
Review comment:
Please add ```final```.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022#discussion_r392141040
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLCredentialsProvider.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Provides an implementation of credentials provider depending on "useMapRSSLConfig" option value
+ * and whether Drill was built with mapr profile or not.
+ */
+abstract class SSLCredentialsProvider {
+
+ private static final String MAPR_CREDENTIALS_PROVIDER_CLIENT = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprClient";
+ private static final String MAPR_CREDENTIALS_PROVIDER_SERVER = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprServer";
+
+ /**
+ * Provides a concrete implementation of {@link SSLCredentialsProvider}.
+ *
+ * @param getPropertyMethod a reference to a method to retrieve credentials from config:
+ * <ul>
+ * <li>String parameter1 - property name</li>
+ * <li>String parameter2 - default value</li>
+ * <li>returns the property value or default value</li>
+ * </ul>
+ * @param mode CLIENT or SERVER
+ * @param useMapRSSLConfig use a MapR credential provider
+ * @return concrete implementation of SSLCredentialsProvider
+ */
+ static SSLCredentialsProvider getSSLCredentialsProvider(BiFunction<String, String, String> getPropertyMethod, SSLConfig.Mode mode, boolean useMapRSSLConfig) {
+ return useMapRSSLConfig ?
+ Optional.ofNullable(getMaprCredentialsProvider(mode))
+ .orElse(new SSLCredentialsProviderImpl(getPropertyMethod)) :
+ new SSLCredentialsProviderImpl(getPropertyMethod);
+ }
+
+ private static SSLCredentialsProvider getMaprCredentialsProvider(SSLConfig.Mode mode) {
+ try {
+ return (SSLCredentialsProvider) Class.forName(getMaprCredentialsProviderClassName(mode)).newInstance();
+ } catch (ClassNotFoundException | InstantiationException | IllegalAccessException e) {
+ // This is expected if built without MapR profile
+ return null;
+ }
+ }
+
+ private static String getMaprCredentialsProviderClassName(SSLConfig.Mode mode) {
Review comment:
there is no need to have a separate method for this, the switch-case can be placed directly into ```getMaprCredentialsProvider(SSLConfig.Mode mode)```.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2022: DRILL-7637: Add an option to retrieve MapR SSL truststore/keystore cr…
URL: https://github.com/apache/drill/pull/2022#discussion_r392139620
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLCredentialsProvider.java
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Provides an implementation of credentials provider depending on "useMapRSSLConfig" option value
+ * and whether Drill was built with mapr profile or not.
+ */
+abstract class SSLCredentialsProvider {
+
+ private static final String MAPR_CREDENTIALS_PROVIDER_CLIENT = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprClient";
+ private static final String MAPR_CREDENTIALS_PROVIDER_SERVER = "org.apache.drill.exec.ssl.SSLCredentialsProviderMaprServer";
+
+ /**
+ * Provides a concrete implementation of {@link SSLCredentialsProvider}.
+ *
+ * @param getPropertyMethod a reference to a method to retrieve credentials from config:
+ * <ul>
+ * <li>String parameter1 - property name</li>
+ * <li>String parameter2 - default value</li>
+ * <li>returns the property value or default value</li>
+ * </ul>
+ * @param mode CLIENT or SERVER
+ * @param useMapRSSLConfig use a MapR credential provider
+ * @return concrete implementation of SSLCredentialsProvider
+ */
+ static SSLCredentialsProvider getSSLCredentialsProvider(BiFunction<String, String, String> getPropertyMethod, SSLConfig.Mode mode, boolean useMapRSSLConfig) {
+ return useMapRSSLConfig ?
+ Optional.ofNullable(getMaprCredentialsProvider(mode))
Review comment:
It would be better to return optional from ```getMaprCredentialsProvider(mode)``` and use ```.orElseGet(supplier)``` to avoid creating everytime instance of ```SSLCredentialsProviderImpl```.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services