You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Parth-Brahmbhatt <gi...@git.apache.org> on 2015/02/27 02:31:55 UTC
[GitHub] storm pull request: STORM-446: Allow superusers to impersonate oth...
GitHub user Parth-Brahmbhatt opened a pull request:
https://github.com/apache/storm/pull/448
STORM-446: Allow superusers to impersonate other users in secure mode.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Parth-Brahmbhatt/incubator-storm STORM-446
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/448.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 #448
----
commit 086e9e588e46922198d56faafbc9d25f4a543f47
Author: Parth Brahmbhatt <br...@gmail.com>
Date: 2015-02-27T01:26:09Z
STORM-446: Allow superusers to impersonate other users in secure mode.
commit 32df28743781767059fc0360c2ef9d166a7af6fb
Author: Parth Brahmbhatt <br...@gmail.com>
Date: 2015-02-27T01:27:02Z
Merge remote-tracking branch 'upstream/master' into STORM-446
----
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76574425
@revans2 I added the fixes. I also added the doAs support to REST API.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76764354
We should fail close, however currently if the acl authorizer is no set we fail open. Not sure why did we make that choice but I did not want alternating behavior. My personal preference is to fail close on any potential security configuration mistake.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-77876005
@Parth-Brahmbhatt Thanks for the quick fix. Ran some tests on the cluster this looks great. +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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/448
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-77773604
@Parth-Brahmbhatt Overall code looks good to me. I am trying to run few tests in secure cluster but during mvn build the following unit test is failing <testcase name="test-populate-req-context-on-null-user" classname="backtype.storm.security.auth.DefaultHttpCredentialsPlugin-test">
<failure>expected: (= 0 (-> handler (.populateContext context req) (.subject) (.getPrincipals) (.size)))
actual: (not (= 0 1))
at: run-test2660255144893935715.clj:85</failure>
</testcase>
Can you please take a look.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76450968
Yes we would need something like that for this to work. I'm not sure I want all administrators to have that power though.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r25526790
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -50,6 +50,7 @@
protected Set<String> _supervisors;
protected IPrincipalToLocal _ptol;
protected IGroupMappingServiceProvider _groupMappingProvider;
+ protected ImpersonationAuthorizer _impersonationAuthorizer;
--- End diff --
Ok, I am going to add this authorization as part of nimbus check-authorization! method. I will also add "nimbus.impersonation.authorizer" config to allow for overriding behavior. I will also add the logging changes there.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r26007092
--- Diff: STORM-UI-REST-API.md ---
@@ -32,6 +32,11 @@ You can use a tool such as `curl` to talk to the REST API:
# Note: We assume ui.port is configured to the default value of 8080.
$ curl http://<ui-host>:8080/api/v1/cluster/configuration
+##Impersonating a user in secure environment
+In a secure environment an authenticated user can impersonate another user. To impersonate a user the caller must pass
+`doAsUser` param or header with value set to the user that the request needs to be performed as. Please see SECURITY.MD
+to learn more about how to setup impersonation ACLs and authorization. The rest API uses the same configs and acls that
+are used by nimbus.
--- End diff --
can you add a curl example 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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76451646
@revans2 I agree. I can allow users to specify regex for matching but that seems like an overkill. For now I am just going to make * as special character for both groups and hosts.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r26008403
--- Diff: STORM-UI-REST-API.md ---
@@ -32,6 +32,11 @@ You can use a tool such as `curl` to talk to the REST API:
# Note: We assume ui.port is configured to the default value of 8080.
$ curl http://<ui-host>:8080/api/v1/cluster/configuration
+##Impersonating a user in secure environment
+In a secure environment an authenticated user can impersonate another user. To impersonate a user the caller must pass
+`doAsUser` param or header with value set to the user that the request needs to be performed as. Please see SECURITY.MD
+to learn more about how to setup impersonation ACLs and authorization. The rest API uses the same configs and acls that
+are used by nimbus.
--- End diff --
Added and upmerged.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r25613390
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -45,28 +45,46 @@
(def ^:dynamic *STORM-CONF* (read-storm-config))
(def ^:dynamic *UI-ACL-HANDLER* (mk-authorization-handler (*STORM-CONF* NIMBUS-AUTHORIZER) *STORM-CONF*))
+(def ^:dynamic *UI-IMPERSONATION-HANDLER* (mk-authorization-handler (*STORM-CONF* NIMBUS-IMPERSONATION-AUTHORIZER) *STORM-CONF*))
(def http-creds-handler (AuthUtils/GetUiHttpCredentialsPlugin *STORM-CONF*))
(defmacro with-nimbus
[nimbus-sym & body]
- `(thrift/with-nimbus-connection
- [~nimbus-sym (*STORM-CONF* NIMBUS-HOST) (*STORM-CONF* NIMBUS-THRIFT-PORT)]
- ~@body))
+ `(let [context# (ReqContext/context)
+ user# (if (.principal context#) (.getName (.principal context#)))]
+ (thrift/with-nimbus-connection-as-user
+ [~nimbus-sym (*STORM-CONF* NIMBUS-HOST) (*STORM-CONF* NIMBUS-THRIFT-PORT) user#]
+ ~@body)))
(defn assert-authorized-user
([servlet-request op]
(assert-authorized-user servlet-request op nil))
([servlet-request op topology-conf]
- (if http-creds-handler (.populateContext http-creds-handler (ReqContext/context) servlet-request))
- (if *UI-ACL-HANDLER*
- (let [context (ReqContext/context)]
- (if-not (.permit *UI-ACL-HANDLER* context op topology-conf)
- (let [principal (.principal context)
- user (if principal (.getName principal) "unknown")]
- (throw (AuthorizationException.
- (str "UI request '" op "' for '"
- user "' user is not authorized")))))))))
+ (let [context (ReqContext/context)]
+ (if http-creds-handler (.populateContext http-creds-handler context servlet-request))
+
+ (if (.isImpersonating context)
+ (if *UI-IMPERSONATION-HANDLER*
+ (if-not (.permit *UI-IMPERSONATION-HANDLER* context op topology-conf)
+ (let [principal (.principal context)
+ real-principal (.realPrincipal context)
+ user (if principal (.getName principal) "unknown")
+ real-user (if real-principal (.getName real-principal) "unknown")
+ remote-address (.remoteAddress context)]
+ (throw (AuthorizationException.
+ (str "user '" real-user "' is not authorized to impersonate user '" user "' from host '" remote-address "'. Please
+ see SECURITY.MD to learn how to configure impersonation ACL.")))))
+ (log-warn " principal " (.realPrincipal context) " is trying to impersonate " (.principal context) " but "
--- End diff --
Again do we want to fail open or fail closed?
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76446301
@revans2 @harshach For supporting the actual jira issue "ui server being able to impersonate users" we probably need to support wildcards ("allow userX to impersonate all users(*) from localhost") or allow the "nimbus.admin" users to impersonate all users from all hosts. Any preferences?
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-77792040
@harshach sorry about that I introduced that failure when I added the rest support and forgot to run the unit tests after that. I have fixed the failing tests.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r25521108
--- Diff: storm-core/src/jvm/backtype/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -50,6 +50,7 @@
protected Set<String> _supervisors;
protected IPrincipalToLocal _ptol;
protected IGroupMappingServiceProvider _groupMappingProvider;
+ protected ImpersonationAuthorizer _impersonationAuthorizer;
--- End diff --
I don't really like the idea of having all authorizers have to be updated to support the impersonation code. I would much rather have the code spliced in at a much higher level that cannot be turned off. Otherwise a custom IAuthorizer instance that does not know to update after this change is now wide open for anyone to impersonate anyone else. The logging changes should also probably happen at the same level.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76749638
For the most part it looks great. Just a little curious if we really want to fail open when no impersonation authorizer is configured? I am +1 either way.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/448#discussion_r25613289
--- Diff: storm-core/src/clj/backtype/storm/daemon/nimbus.clj ---
@@ -778,9 +779,22 @@
(defn check-authorization!
([nimbus storm-name storm-conf operation context]
(let [aclHandler (:authorization-handler nimbus)
+ impersonation-authorizer (:impersonation-authorization-handler nimbus)
ctx (or context (ReqContext/context))
check-conf (if storm-conf storm-conf (if storm-name {TOPOLOGY-NAME storm-name}))]
(log-message "[req " (.requestID ctx) "] Access from: " (.remoteAddress ctx) " principal:" (.principal ctx) " op:" operation)
+
+ (if (.isImpersonating ctx)
+ (do
+ (log-warn "principal: " (.realPrincipal ctx) " is trying to impersonate principal: " (.principal ctx))
+ (if impersonation-authorizer
+ (if-not (.permit impersonation-authorizer ctx operation check-conf)
+ (throw (AuthorizationException. (str "principal " (.realPrincipal ctx) " is not authorized to impersonate
+ principal " (.principal ctx) " from host " (.remoteAddress ctx) " Please see SECURITY.MD to learn
+ how to configure impersonation acls."))))
+ (log-warn "impersonation attempt but " NIMBUS-IMPERSONATION-AUTHORIZER " has no authorizer configured. potential
--- End diff --
Do we want to fail open or fail closed in this case. For me if someone tries to do impersonation and we have not configured anything for this I would rather fail closed.
---
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] storm pull request: STORM-446: Allow superusers to impersonate oth...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/448#issuecomment-76430788
I haven't gone deeply into the code, but for the most part it looks good. Just one request that should hopefully not be too difficult to 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.
---