-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-1320] Run zeppelin interpreter process as web front end user #1322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
shouldn't interpreter process be impersonating the user logging onto the web front end? |
|
@felixcheung Fair point, let me try and do it, will change the title to WIP for now. |
|
|
||
| private Interpreter getInterpreter(String noteId, InterpreterSetting setting, String name) { | ||
| private Interpreter getInterpreter(String noteId, InterpreterSetting setting, String name, | ||
| String userName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a nitpick, but horizontal alignment is controversial idea and generally is discouraged by the styleguide as it creates a "blast radius" of re-formatting in case of future changes i.e renaming a function.
# Conflicts: # zeppelin-web/src/app/interpreter/interpreter.controller.js # zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java # zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterOption.java
f449fc0 to
590032e
Compare
cd62bc5 to
787a108
Compare
|
CI green! Ready for review. |
ac00c5f to
55ccce3
Compare
| u) | ||
| ZEPPELIN_SSH_COMMAND="ssh ${OPTARG}@localhost " | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires the login user must exist in the os account and be able to ssh to localhost. I am not sure whether this is a good way, but just feel the approach is a little strange compared to the impersonation implementation in hadoop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjffdu yes, I agree, its not as implementation in hadoop, would you recommend something else ?
|
I agree that it's simple way to use ssh to support impersonation. but I'm worried about it. First, we should consider not to use ssh server in a local machine. It's disabled on Mac by default and in case of Windows users, they might not have any ssh server. Second, even if all of users can use connect their machine via ssh, all of users' name should be the same as system users. AFAIK, Some Zeppelin use cases, the system admin uses virtual users as well. Do you think of it? |
|
Yes, I thought about the usage in mac and windows, and initially started of with using Have not thought about the cases in which system admin uses virtual users. Now since with this, we are able to propagate end web user to RemoteInterpreterManagedProcess.start, we can choose to use some other mechanism in What do you recommend, that would be a secure and all full proof mechanism by which we can run interpreter as different user ? |
|
|
||
| import com.google.gson.Gson; | ||
| import com.google.gson.reflect.TypeToken; | ||
| import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a nitpick but Zeppelin's Java code conventions discourages usage of wildcard imports.
Could you please check all the other changed files to follow this convention as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll revert this, and check that my Editor (Intellij Idea) is also configured properly.
|
@prabhjyotsingh I don't know how to support different users' environments fully, actually. But I think it's better to use |
|
@prabhjyotsingh Without issues above, Could you check this PR support |
|
If i add one more, That'll give user flexibility of selecting current behavior (without impersonation) and new behavior. Otherwise, this PR will make incompatible user behavior change. |
@jongyoul How about I make
Yes I've checked this with Shell and Python interpreter it was working as expected. @Leemoonsoo, yes agreed, I too think this options should be there, and have implemented it as well. If you take a look at GIF attached in this PR description, it's doing that you are asking for :) |
|
Whatever I guess we all agree and are aware that adding user We should make this clear in the doc but also stress it in the UI (with a hover, or a clear text/link near the User Impersonate. |
|
@echarles , Yes agreed, will need to update in doc, and a extra toolbar near the check box where user can enable User Impersonate. |
|
To make ZEPPELIN-1337 Umbrella for multiple user support for zeppelin more readable, should we rename the following:
|
|
... and make ZEPPELIN-1320 a subtask of ZEPPELIN-1337 ? |
|
Yes, you are right, let me do it right away. |
|
@prabhjyotsingh I agree @echarles's idea. Interpreter tries to find hadoop dependencies first and if it passes, it uses |
|
Sure, In this PR I was only thinking about the otherwise case i.e. in the environment where hadoop dependencies where not present, and hence start interpreter as end-web-user. |
|
Btw, for the hadoop case (or spark on yarn case), this PR may give an issue for Typically, you configure If we run ssh/su as the front-end user, we will not fullfill what the hadoop/yarn cluster is expecting. We thus should have two checkboxes:
If you select one, I would expect the other one to be disabled. |
|
Agreed @echarles, the |
|
Instead of by default. but user can override this env variable, like It gives more flexibility i think. (e.g. give additional options like -p. use different command to impersonate) |
|
@Leemoonsoo yes thats a good suggestion. Let me try and do it. |
|
I got following checkstyle error while building source. @prabhjyotsingh Could you fix this? |
|
Closing this, will open a new one with merge of #1265. |
Have recreated this from #1322 ### What is this PR for? While running a Notebook using shell, spark, python uses same user as which zeppelin server is running. Which means these interprets have same permission on file system as zeppelin server. IMO users should be able to impersonate themselves as a complete security system. ### What type of PR is it? [Improvement] ### Todos - [x] - Update doc - [x] - FIX NPEs - [x] - FIX CI ### What is the Jira issue? - [ZEPPELIN-1320](https://issues.apache.org/jira/browse/ZEPPELIN-1320) ### How should this be tested? - Enable shiro auth in shiro.ini - Add ssh key for the same user you want to try and impersonate (say user1). ``` adduser user1 ssh-keygen ssh user1localhost mkdir -p .ssh cat ~/.ssh/id_rsa.pub | ssh user1localhost 'cat >> .ssh/authorized_keys' ``` - Start zeppelin server, try and run following in paragraph in a notebook - Go to interpreter setting page, and enable "User Impersonate" in any of the interpreter (in my example its shell interpreter) ``` %sh whoami ``` Check that it should run as new user, i.e. "user1" ### Screenshots (if appropriate)  ### Questions: - Does the licenses files need update? no - Is there breaking changes for older versions? no - Does this needs documentation? yes Author: Prabhjyot Singh <[email protected]> Closes #1554 from prabhjyotsingh/ZEPPELIN-1320-2 and squashes the following commits: dc69c9d [Prabhjyot Singh] @Leemoonsoo review comment: making ZEPPELIN_SSH_COMMAND configurable 1b26cc0 [Prabhjyot Singh] add doc 5a76839 [Prabhjyot Singh] show User Impersonate only when interpreter setting is "per user" and "isolated" 02c3084 [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1320-2 03b2f20 [Prabhjyot Singh] use user instead of "" 0ff80ec [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1320-2 dd0731d [Prabhjyot Singh] fix missing test cases aff1bf0 [Prabhjyot Singh] user should have option to run these interpreters as different user.
|
Sorry for late comment. I was in vacation in the last 2 weeks. I found this didn't work for spark interpreter. @prabhjyotsingh Did you try it for spark interpreter and other interpreters ? |
|
@zjffdu Yes, you are right, with SPARK_HOME/SPARK_SUBMIT it doesn't work. |
|
Then I think we should either revert this PR or fix it for spark interpreter as well. Because spark interpreter is the most important interpreter of zeppelin IMO. |
|
Sure make sense I'll try to fix it ASAP. https://issues.apache.org/jira/browse/ZEPPELIN-1701 |
Have recreated this from apache#1322 ### What is this PR for? While running a Notebook using shell, spark, python uses same user as which zeppelin server is running. Which means these interprets have same permission on file system as zeppelin server. IMO users should be able to impersonate themselves as a complete security system. ### What type of PR is it? [Improvement] ### Todos - [x] - Update doc - [x] - FIX NPEs - [x] - FIX CI ### What is the Jira issue? - [ZEPPELIN-1320](https://issues.apache.org/jira/browse/ZEPPELIN-1320) ### How should this be tested? - Enable shiro auth in shiro.ini - Add ssh key for the same user you want to try and impersonate (say user1). ``` adduser user1 ssh-keygen ssh user1localhost mkdir -p .ssh cat ~/.ssh/id_rsa.pub | ssh user1localhost 'cat >> .ssh/authorized_keys' ``` - Start zeppelin server, try and run following in paragraph in a notebook - Go to interpreter setting page, and enable "User Impersonate" in any of the interpreter (in my example its shell interpreter) ``` %sh whoami ``` Check that it should run as new user, i.e. "user1" ### Screenshots (if appropriate)  ### Questions: - Does the licenses files need update? no - Is there breaking changes for older versions? no - Does this needs documentation? yes Author: Prabhjyot Singh <[email protected]> Closes apache#1554 from prabhjyotsingh/ZEPPELIN-1320-2 and squashes the following commits: dc69c9d [Prabhjyot Singh] @Leemoonsoo review comment: making ZEPPELIN_SSH_COMMAND configurable 1b26cc0 [Prabhjyot Singh] add doc 5a76839 [Prabhjyot Singh] show User Impersonate only when interpreter setting is "per user" and "isolated" 02c3084 [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1320-2 03b2f20 [Prabhjyot Singh] use user instead of "" 0ff80ec [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1320-2 dd0731d [Prabhjyot Singh] fix missing test cases aff1bf0 [Prabhjyot Singh] user should have option to run these interpreters as different user.
What is this PR for?
While running a Notebook using shell, spark, python uses same user as which zeppelin server is running. Which means these interprets have same permission on file system as zeppelin server.
IMO users should be able to impersonate themselves as a complete security system.
What type of PR is it?
[Improvement]
Todos
What is the Jira issue?
How should this be tested?
Check that it should run as new user, i.e. "user1"
Screenshots (if appropriate)
Questions: