You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sohami <gi...@git.apache.org> on 2017/07/07 01:26:51 UTC
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
GitHub user sohami opened a pull request:
https://github.com/apache/drill/pull/870
DRILL-5664: Enable security for Drill HiveStoragePlugin based on a co…
…nfig parameter
Change to enable/disable HiveStoragePlugin security configuration based on Drill's "security.storage_plugin.enabled" configuration. This will help to open secure channel between Drill's HiveClient and HiveMetastore
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sohami/drill DRILL-5664
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/870.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 #870
----
commit 52618f7d319b9b9314a25e3bad872453be19d04d
Author: Sorabh Hamirwasia <sh...@maprtech.com>
Date: 2017-06-21T01:26:06Z
DRILL-5664: Enable security for Drill HiveStoragePlugin based on a config parameter
Change to enable/disable HiveStoragePlugin security configuration based on Drill's "security.storage_plugin.enabled" configuration. This will help to open secure channel between Drill's HiveClient and HiveMetastore
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #870: DRILL-5664: Enable security for Drill HiveStoragePlugin ba...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/870
closing this PR pending the revised version.
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/870#discussion_r126839078
--- Diff: contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveStoragePluginConfig.java ---
@@ -33,6 +33,8 @@
public static final String NAME = "hive";
+ public static final String HIVE_SECURITY_CONFIG = "hive.metastore.sasl.enabled";
--- End diff --
Fine to define this config option here. But, since this is Hive, and Hive has its own `drill-module.conf` file, please provide a default option value in that file. (The config system will throw an exception if the requested option is undefined.)
Then, if you expect users to change this, please provide an example and description in `drill-override-example.conf`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #870: DRILL-5664: Enable security for Drill HiveStoragePlugin ba...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/870
What is the status of this PR? Appears that there was substantial discussion about whether the approach here is appropriate for Apache Drill. Had the PR been updated with an alternative? Or, should this PR be closed pending a revised solution?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #870: DRILL-5664: Enable security for Drill HiveStoragePlugin ba...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/870
I have a question. The user can change the storage plugin configuration itself instead of enabling/disabling this option. what is the use of this option ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/870#discussion_r126854772
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java ---
@@ -260,6 +289,10 @@ public StoragePlugin createOrUpdate(String name, StoragePluginConfig config, boo
if (done) {
if (persist) {
+ if (!config.isValidSecurityConfig(secureStoragePlugin)) {
+ logger.warn("Security setting for {} plugin is not recommended one. It should be set to: {}",
--- End diff --
This will cause confusion if we let the user update the storage plugin config ignoring the drill security config. I agree with Paul this can be messy, with things getting out of sync. We should discuss about this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/870#discussion_r126839511
--- Diff: contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveStoragePluginConfig.java ---
@@ -66,4 +68,40 @@ public boolean equals(Object o) {
return true;
}
+
+ @Override
+ public boolean enableSecurity() {
+ return compareAndUpdateSecurityConfig("true");
+ }
+
+ @Override
+ public boolean disableSecurity() {
+ return compareAndUpdateSecurityConfig("false");
+ }
+
+ /**
+ * Compare the newValue with current value of security setting for Hive Plugin
+ *
+ * @param newValue - new value for security setting.
+ * @return true - if modified
+ * false - if not modified
+ */
+ public boolean compareAndUpdateSecurityConfig(String newValue) {
--- End diff --
The `compare` part is an implementation detail. Maybe just call it `updateSecurity()`.
Then, passing in strings seems strange. Maybe pass in a boolean. Then, in this method, do `Boolean.toString()` to get the string value.
Finally, why even bother to check fi the value exists? Why not just put the new value and return it (since it will be a boolean)?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by sohami <gi...@git.apache.org>.
Github user sohami closed the pull request at:
https://github.com/apache/drill/pull/870
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/870#discussion_r126840721
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java ---
@@ -146,23 +148,37 @@ public void init() throws DrillbitStartupException {
String pluginsData = Resources.toString(url, Charsets.UTF_8);
StoragePlugins plugins = lpPersistence.getMapper().readValue(pluginsData, StoragePlugins.class);
for (Map.Entry<String, StoragePluginConfig> config : plugins) {
- if (!definePluginConfig(config.getKey(), config.getValue())) {
+
+ final String pluginName = config.getKey();
+ final StoragePluginConfig pluginConfig = config.getValue();
+
+ if (!definePluginConfig(pluginName, pluginConfig)) {
logger.warn("Duplicate plugin instance '{}' defined in [{}, {}], ignoring the later one.",
- config.getKey(), pluginURLMap.get(config.getKey()), url);
+ config.getKey(), pluginURLMap.get(pluginName), url);
continue;
}
- pluginURLMap.put(config.getKey(), url);
+ pluginURLMap.put(pluginName, url);
}
}
} else {
throw new IOException("Failure finding " + ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE);
}
}
- Map<String, StoragePlugin> activePlugins = new HashMap<String, StoragePlugin>();
+ final Map<String, StoragePlugin> activePlugins = new HashMap<String, StoragePlugin>();
for (Map.Entry<String, StoragePluginConfig> entry : Lists.newArrayList(pluginSystemTable.getAll())) {
- String name = entry.getKey();
- StoragePluginConfig config = entry.getValue();
+ final String name = entry.getKey();
+ final StoragePluginConfig config = entry.getValue();
+
+ // Update the security setting inside StoragePluginConfig based on secureStoragePlugin flag
--- End diff --
This doesn't seem right at all... We are updating in ZK the value of a plugin property based on something defined in config. All this can ever be is wrong an out-of-date. If the user changes the plugin after start, do we honor the boot option or the storage plugin option? Will they understand when we change the option back on next restart? A big mess...
Better: add a new `configure(PluginContext context)` method to the base plugin class. One hopes that there is an abstract base class that can provide the default implementation.
Then, define the `PluginContext` class with information of interest to the plugins. This might include the `DrillConfig` and your security setting.
Plugins are then obligated to use the provided value; not one stored in the plugin config.
But, this has a separate problem. What if I have two Hive servers: one which is secure, and one which is open? The same rule must be applied to both. So, this argument says that security of a Hive server is *not* a Drill boot-time config setting, it is instead and attribute of the remote system and *should* be part of the storage plugin config, and should *not* be altered by Drill.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #870: DRILL-5664: Enable security for Drill HiveStoragePlugin ba...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/870
More details in JIRA [DRILL-5664](https://issues.apache.org/jira/browse/DRILL-5664).
@parthchandra / @ppadma - Can you please help to review this change ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #870: DRILL-5664: Enable security for Drill HiveStoragePlugin ba...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/870
@ppadma -
1) For the ease of use. Once the security is being configured for Drill then it's better if all the config changes are done in single place (which is drill-override.conf) and then make sure that all communications are secure. With this option user doesn't have to explicitly go and edit the Storage Plugin configuration to enable security.
2) It will take care of enabling/disabling security both for active and inactive plugins.
3) This can be extended for handling security of all the storage plugin. Hence this config can be a single place to enable/disable security for all the plugins in one go rather than doing it manually for each and every plugin one by one.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #870: DRILL-5664: Enable security for Drill HiveStoragePl...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/870#discussion_r126840055
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -155,6 +155,10 @@ drill.exec: {
enabled: false,
max_chained_user_hops: 3
},
+
+ // Setting to control security for storage_plugins. For now this is only for Hive
--- End diff --
Better:
```
security: {
user.auth.enabled: false,
storage_plugin_enabled: false
}
```
That way, it will be clear that the two properties go together. Move your comment inside the brackets.
Now, it is a hassle that some previous properties used dots as word separators: they are actually group separators...
Then, if users must set this option, please add this property as an example in `drill-override-example.conf`. Provide a user-understandable instruction for what to do. Also, please mark this JIRA as doc-impacting if it is not already.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---