You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by harshach <gi...@git.apache.org> on 2014/06/27 00:07:03 UTC
[GitHub] incubator-storm pull request: STORM-347. (Security) authentication...
GitHub user harshach opened a pull request:
https://github.com/apache/incubator-storm/pull/166
STORM-347. (Security) authentication should allow for groups not just users.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/harshach/incubator-storm STORM-347
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/166.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 #166
----
commit f3112fa76e98038da83170513a476499dde0bb41
Author: Sriharsha Chintalapani <ma...@harsha.io>
Date: 2014-06-26T22:05:21Z
STORM-347. (Security) authentication should allow for groups not just users.
----
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/166#discussion_r14966233
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/ShellBasedUnixGroupsMapping.java ---
@@ -0,0 +1,88 @@
+/**
+ * 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 backtype.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.StringTokenizer;
+import backtype.storm.utils.ShellUtils;
+import backtype.storm.utils.ShellUtils.ExitCodeException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ShellBasedUnixGroupsMapping implements
+ IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);
+
+ /**
+ * Invoked once immediately after construction
+ * @param storm_conf Storm configuration
+ */
+ public void prepare(Map storm_conf) {}
--- End diff --
Could you add @Override annotations, and unless the javadocs are different from the interface they override are really not needed here.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/166#discussion_r14966175
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/IGroupMappingServiceProvider.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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 backtype.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.Map;
+
+public interface IGroupMappingServiceProvider {
+
+ /**
+ * Invoked once immediately after construction
+ * @param storm_conf Storm configuration
+ */
+ void prepare(Map storm_conf);
+
+ /**
+ * Get all various group memberships of a given user.
+ * Returns EMPTY list in case of non-existing user
+ * @param user User's name
+ * @return group memberships of user
+ * @throws IOException
+ */
+ public Set<String> getGroups(String user) throws IOException;
+
+ /**
+ * Refresh the cache of groups and user mapping
+ * @throws IOException
+ */
+ public void cacheGroupsRefresh() throws IOException;
+ /**
--- End diff --
Add a space above (very minor)
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-49674242
@revans2 I added caching to ShellBasedUnixGroupsMapping class using TimeCacheMap with a config option. I removed caching related apis from the interface as you suggested didn't feel they are useful while implementing the above caching.
Is it ok if I do the merge for ShellProcess and ShellUtils in another 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-50408018
The change looks good to me. Let me run through the tests and if it all checks out I am +1.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/166#discussion_r15058887
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/ShellBasedUnixGroupsMapping.java ---
@@ -0,0 +1,88 @@
+/**
+ * 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 backtype.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.StringTokenizer;
+import backtype.storm.utils.ShellUtils;
+import backtype.storm.utils.ShellUtils.ExitCodeException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ShellBasedUnixGroupsMapping implements
+ IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);
+
+ /**
+ * Invoked once immediately after construction
+ * @param storm_conf Storm configuration
+ */
+ public void prepare(Map storm_conf) {}
+
+ /**
+ * Returns list of groups for a user
+ *
+ * @param user get groups for this user
+ * @return list of groups for a given user
+ */
+ @Override
+ public Set<String> getGroups(String user) throws IOException {
+ return getUnixGroups(user);
--- End diff --
Right now you are probably correct, but I was thinking about DRPC. I would like to add this to the DRPC authorization, and that would require caching. So lets leave it for now, and I'll file a JIRA for adding groups support to DRPC, and include some caching in there 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-49099439
For the most part the code looks really good. I am concerned about not having any caching in the groups implementation, even if it expires after just a couple of seconds. I also don't think we need to expose the caching APIs explicitly. They can probably just be an implementation detail. If you want to put in the caching in a follow on JIRA that is fine too. Getting the API in place is probably the most critical.
I am fine with you borrowing some code from Hadoop to make this simpler. No need to reinvent the wheel. But there are already some Shell utilities in storm that do similar things.
https://github.com/apache/incubator-storm/blob/master/storm-core/src/jvm/backtype/storm/utils/ShellProcess.java
It would be good to file a follow on JIRA to look at how we can combine the two together so we don't have two classes that do almost the same thing.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/166#discussion_r14966321
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/ShellBasedUnixGroupsMapping.java ---
@@ -0,0 +1,88 @@
+/**
+ * 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 backtype.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.StringTokenizer;
+import backtype.storm.utils.ShellUtils;
+import backtype.storm.utils.ShellUtils.ExitCodeException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ShellBasedUnixGroupsMapping implements
+ IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);
+
+ /**
+ * Invoked once immediately after construction
+ * @param storm_conf Storm configuration
+ */
+ public void prepare(Map storm_conf) {}
+
+ /**
+ * Returns list of groups for a user
+ *
+ * @param user get groups for this user
+ * @return list of groups for a given user
+ */
+ @Override
+ public Set<String> getGroups(String user) throws IOException {
+ return getUnixGroups(user);
--- End diff --
This is likely to be called very frequently and fork/exec are not fast. We should implement some of the caching the Interface suggests should be going on.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-50390090
Thanks @revans2 . I added a simple unit test and dropped the "unix" from the file name. Please check.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/166
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-49310889
OK I am fine with leaving the APIs there, if you have plans to use them in the near future, especially with Active directory for windows support.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-49237484
@revans2 Thanks for the feedback. Regarding caching methods in the interface these are provided for potential plugins which might be fetching group info from ldap or active directory and can add caching to those calls.
Regarding ShellProcess I will work in integrating that into ShellUtils.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-50172155
Yes, feel free to do the merge on another JIRA. The code looks good. There are a lot of white space changes to Config.java, but I don't think that is too critical.
I would like to see ShellBasedUnixGroupsMapping drop the Unix in it's name. Looks like it will work with Windows too. Also it would be good to have some tests for this code. I am fine with some simple unit tests that look at the groups code in isolation. I realize it may be hard to do this cleanly for Windows/Unix etc, especially when you don't know what user is running the tests. But perhaps just a sanity test that you can get something back without it blowing up.
---
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] incubator-storm pull request: STORM-347. (Security) authentication...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/166#discussion_r15032581
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/ShellBasedUnixGroupsMapping.java ---
@@ -0,0 +1,88 @@
+/**
+ * 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 backtype.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.StringTokenizer;
+import backtype.storm.utils.ShellUtils;
+import backtype.storm.utils.ShellUtils.ExitCodeException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ShellBasedUnixGroupsMapping implements
+ IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);
+
+ /**
+ * Invoked once immediately after construction
+ * @param storm_conf Storm configuration
+ */
+ public void prepare(Map storm_conf) {}
+
+ /**
+ * Returns list of groups for a user
+ *
+ * @param user get groups for this user
+ * @return list of groups for a given user
+ */
+ @Override
+ public Set<String> getGroups(String user) throws IOException {
+ return getUnixGroups(user);
--- End diff --
This plugin will be called Authorizer.permit method which is only get checked if there is any operation being performed on the cluster or topology. I don't think it will be that frequent that it requires caching. If you think otherwise let me know I'll add the caching.
---
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.
---