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/08/05 04:22:16 UTC
[GitHub] incubator-storm pull request: STORM-349. (Security) ui actions sho...
GitHub user harshach opened a pull request:
https://github.com/apache/incubator-storm/pull/215
STORM-349. (Security) ui actions should have nimbus like authroization.
Upgraded ring to 1.3.0. Added IAuthorizer instance to UI daemon. Added
hadoop-auth as a dependency which allow users to configure
AuthenticationFilter.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/harshach/incubator-storm STORM-349-V2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/215.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 #215
----
commit 3208a8e3f39140227fe6be9c686b795a19777e64
Author: Sriharsha Chintalapani <ma...@harsha.io>
Date: 2014-08-05T02:18:26Z
STORM-349. (Security) ui actions should have nimbus like authroization.
Upgraded ring to 1.3.0. Added IAuthorizer instance to UI daemon. Added
hadoop-auth as a dependency which allow users to configure
AuthenticationFilter.
----
---
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-349. (Security) ui actions sho...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/215#issuecomment-52522747
Just to add more info what we are using from hadoop-auth is AuthenticationFilter and KerberosAuthenticationHandler. Apart from these excluded all the dependencies from hadoop-auth package.
---
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-349. (Security) ui actions sho...
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/215#discussion_r15882654
--- Diff: storm-core/pom.xml ---
@@ -197,6 +201,21 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-auth</artifactId>
--- End diff --
Like I said I am OK with adding in new dependencies, I would just like to better understand in full detail exactly what is being pulled in, and if we can leave any of it out through exclusions. STORM-129 tracks isolating user and system dependencies which is where an ultimate solution really needs to come from. For the time being I just want to keep unnecessary changes to a minimum.
---
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-349. (Security) ui actions sho...
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/215#discussion_r15836462
--- Diff: storm-core/pom.xml ---
@@ -197,6 +201,21 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-auth</artifactId>
--- End diff --
I'm not sure we want to pull this in directly to storm itself. Storm already has a lot of dependencies and adding more seems to me like it could pollute the user classpath. Looking at https://github.com/apache/hadoop-common/blob/trunk/hadoop-common-project/hadoop-auth/pom.xml it looks like this is going to add in javax.servlet:servlet-api, commons-codec:commons-codec, org.apache.directory.server:apacheds-kerberos-codec and org.apache.hadoop:hadoop-annotations to the classpath. Possibly along with some oddness with two versions of Jetty. org.mortbay.jetty:jetty and the org.eclipse.jetty:jetty that you added in above too. I am OK with adding in new dependencies, but I would like to understand better what really needs to be here and what does not.
Ideally it would also be nice to separate out dependencies that are needed for worker processes from everything else. But that is probably a separate 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-349. (Security) ui actions sho...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/215#issuecomment-52241834
@revans2 I excluded almost all of the dependencies from hadoop-auth and removed Config.UI_ACTIONS_ENABLED. Please check and let me know if it looks good. Thanks
---
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-349. (Security) ui actions sho...
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/215#discussion_r16118504
--- Diff: storm-core/pom.xml ---
@@ -197,6 +201,21 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-auth</artifactId>
--- End diff --
@revans2 I pulled out of all the dependencies from hadoop-auth as most of them are already in storm-core/pom.xml or not needed. I noticed a bug when user doesn't have UI-Filter configured it will throw an exception as servlet-request doesn't contain any user related info which causes authorizer to fail. I'll update the PR. Thanks.
---
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-349. (Security) ui actions sho...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/215#issuecomment-51250810
Overall it looks good. I think in the future we probably want to explore allowing the UI to pretend to be a different user to nimbus. This would make the code a lot simpler, and would open up a number of other possibilities in the future.
We could try to do this by modifying the SASL negotiation that happens when authenticating the connection, but I think it would be simpler to update the different Thrift operations to optionally include a doAs argument. Then Nimbus would validate that the real user is authorized to perform operations on behalf of other users and update things accordingly. Perhaps you could file a JIRA for 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] incubator-storm pull request: STORM-349. (Security) ui actions sho...
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/215#discussion_r15837693
--- Diff: storm-core/src/clj/backtype/storm/ui/helpers.clj ---
@@ -23,7 +23,8 @@
(:use [backtype.storm.util :only [clojurify-structure uuid defnk url-encode]])
(:use [clj-time coerce format])
(:import [backtype.storm.generated ExecutorInfo ExecutorSummary])
- (:import [org.mortbay.jetty.security SslSocketConnector])
+ (:import [org.eclipse.jetty.server.ssl SslSocketConnector]
--- End diff --
@revans2 Yes upgraded the ring to 1.3.0 which uses jetty-7
---
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-349. (Security) ui actions sho...
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/215#discussion_r15836939
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -47,33 +52,19 @@
[~nimbus-sym (*STORM-CONF* NIMBUS-HOST) (*STORM-CONF* NIMBUS-THRIFT-PORT)]
~@body))
-(defn authorized-ui-user?
- [user conf topology-conf]
- (let [ui-users (concat (conf UI-USERS)
- (conf NIMBUS-ADMINS)
- (topology-conf UI-USERS)
- (topology-conf TOPOLOGY-USERS))]
- (or (blank? (conf UI-FILTER))
- (and (not (blank? user))
- (some #(= % user) ui-users)))))
-
-(defn assert-authorized-ui-user
- [user conf topology-conf]
- (if (not (authorized-ui-user? user conf topology-conf))
- ;;TODO need a better exception here so the UI can appear better
- (throw (RuntimeException. (str "User " user " is not authorized.")))))
-
-(defn- ui-actions-enabled?
- []
- (= "true" (lower-case (*STORM-CONF* UI-ACTIONS-ENABLED))))
--- End diff --
We should remove Config.UI_ACTIONS_ENABLED, or at least deprecate it.
---
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-349. (Security) ui actions sho...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/215
---
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-349. (Security) ui actions sho...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/215#issuecomment-52541263
Looks good. I did a diff of the dependencies with what is currently on master and I saw both Jetty and ring change and hadoop-auth added in.
I saw java.classpath-0.2.2.jar and tools.namespace-0.2.4.jar come in too, they seem to be coming with the ring update, so I think it is OK. 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-349. (Security) ui actions sho...
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/215#discussion_r15837836
--- Diff: storm-core/pom.xml ---
@@ -197,6 +201,21 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-auth</artifactId>
--- End diff --
@revans2 I understand having that as dependency is going to be issue. But wanted to have kerberos based filter to be shipped with storm it will give users an easy access instead of putting hadoop-auth in the classpath to enable the filter. What you think about copying required code from hadoop-auth into storm. Mostly duplicating as there wasn't anything to chnage apart from probably cookie 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-349. (Security) ui actions sho...
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/215#discussion_r15836803
--- Diff: storm-core/src/clj/backtype/storm/ui/helpers.clj ---
@@ -23,7 +23,8 @@
(:use [backtype.storm.util :only [clojurify-structure uuid defnk url-encode]])
(:use [clj-time coerce format])
(:import [backtype.storm.generated ExecutorInfo ExecutorSummary])
- (:import [org.mortbay.jetty.security SslSocketConnector])
+ (:import [org.eclipse.jetty.server.ssl SslSocketConnector]
--- End diff --
So can I assume then, that this also switched the version of jetty that we are using to 7.X from the 6.x line?
---
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-349. (Security) ui actions sho...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/incubator-storm/pull/215#issuecomment-51251797
@revans2 Thanks for the feedback. I'll file a jira for doAs.
---
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.
---