[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r255161959 ## File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/LivyThriftSessionManager.scala ## @@ -230,6 +230,7 @@ class LivyThriftSessionManager(val server: LivyThriftServer, val livyConf: LivyC server.livySessionManager.nextId(), None, username, +None, Review comment: Interesting; but at least now, it doesn't seem like that information is being reflected in Livy's state. Shouldn't matter much, but it's something to keep in mind. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254798654 ## File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/LivyThriftSessionManager.scala ## @@ -230,6 +230,7 @@ class LivyThriftSessionManager(val server: LivyThriftServer, val livyConf: LivyC server.livySessionManager.nextId(), None, username, +None, Review comment: Hmm, looks like the thrift side doesn't yet handle impersonation, so this is fine. Would probably be good to have a bug for that. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254366621
##
File path:
server/src/test/scala/org/apache/livy/server/SessionServletSpec.scala
##
@@ -114,17 +111,66 @@ class SessionServletSpec extends
BaseSessionServletSpec[Session, RecoveryMetadat
}
it("should attach owner information to sessions") {
+ jpost[MockSessionView]("/", Map()) { res =>
+assert(res.owner === null)
+assert(res.proxyUser === None)
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, adminHeaders, SC_OK)
+ }
+
jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
assert(res.owner === "alice")
+assert(res.proxyUser === Some("alice"))
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/?doAs=alice", Map(), headers = adminHeaders) {
res =>
+assert(res.owner === ADMIN)
+assert(res.proxyUser === Some("alice"))
assert(res.logs === IndexedSeq("log"))
delete(res.id, aliceHeaders, SC_OK)
}
}
it("should allow other users to see non-sensitive information") {
+ jpost[MockSessionView]("/", Map()) { res =>
+jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
+ assert(res.owner === null)
+ assert(res.proxyUser === None)
+ assert(res.logs === IndexedSeq("log"))
Review comment:
I'm fine with the "behavior changes in another PR" argument. But the test
name here is so obviously wrong, that if someone else ends up touching this,
they'll be scratching their heads.
So at the very least, the test name must be fixed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254107884
##
File path:
server/src/test/scala/org/apache/livy/server/interactive/JobApiSpec.scala
##
@@ -227,3 +230,59 @@ class JobApiSpec extends BaseInteractiveServletSpec {
}
}
+
+class JobApiSpecNoImpersonation extends JobApiSpec {
+ override protected def createConf(): LivyConf = synchronized {
+super.createConf().set(LivyConf.IMPERSONATION_ENABLED, false)
+ }
+
+ it("should not support user impersonation") {
+assume(!createConf().getBoolean(LivyConf.IMPERSONATION_ENABLED))
+jpost[SessionInfo]("/", createRequest(inProcess = false)) { data =>
+ try {
+waitForIdle(data.id)
+data.owner should be (null)
+data.proxyUser should be (null)
+val user = runJob(data.id, new GetCurrentUser())
+user should be (UserGroupInformation.getCurrentUser.getUserName)
+ } finally {
+deleteSession(data.id)
+ }
+}
+
+val headers = makeUserHeaders(PROXY)
+jpost[SessionInfo]("/", createRequest(inProcess = false), headers =
headers) { data =>
+ try {
+waitForIdle(data.id)
+data.owner should be (PROXY)
+data.proxyUser should be (null)
+val user = runJob(data.id, new GetCurrentUser(), headers = headers)
+user should be (UserGroupInformation.getCurrentUser.getUserName)
+ } finally {
+deleteSession(data.id)
+ }
+}
+ }
+
+ it("should not honor impersonation requests") {
Review comment:
I'm not sure I follow these tests. What does it mean to not honor
impersonation requests?
I'd expect that if impersonation is disabled and you ask for it, you'd get
an error. That makes more sense to me than falling back to some other mode.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254108314
##
File path:
server/src/test/scala/org/apache/livy/server/SessionServletSpec.scala
##
@@ -114,17 +111,66 @@ class SessionServletSpec extends
BaseSessionServletSpec[Session, RecoveryMetadat
}
it("should attach owner information to sessions") {
+ jpost[MockSessionView]("/", Map()) { res =>
+assert(res.owner === null)
+assert(res.proxyUser === None)
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, adminHeaders, SC_OK)
+ }
+
jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
assert(res.owner === "alice")
+assert(res.proxyUser === Some("alice"))
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/?doAs=alice", Map(), headers = adminHeaders) {
res =>
+assert(res.owner === ADMIN)
+assert(res.proxyUser === Some("alice"))
assert(res.logs === IndexedSeq("log"))
delete(res.id, aliceHeaders, SC_OK)
}
}
it("should allow other users to see non-sensitive information") {
+ jpost[MockSessionView]("/", Map()) { res =>
+jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
+ assert(res.owner === null)
+ assert(res.proxyUser === None)
+ assert(res.logs === IndexedSeq("log"))
Review comment:
Sorry, but this is really bothering me.
The test says "should allow other users to see non-sensitive information",
and logs are sensitive information. So if "bob" is not the owner, and is not a
super user, then either this test is wrong or the test name is wrong.
Seems like no security is enabled in these tests, so basically everyone has
access to everything, so the test name should reflect that.
And if it's true that no security is enabled here, I'm not sure what's the
point of adding impersonation-related tests.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r253705439
##
File path:
server/src/test/scala/org/apache/livy/server/SessionServletSpec.scala
##
@@ -112,17 +109,60 @@ class SessionServletSpec extends
BaseSessionServletSpec[Session, RecoveryMetadat
}
it("should attach owner information to sessions") {
+ jpost[MockSessionView]("/", Map()) { res =>
+assert(res.owner === null)
+assert(res.proxyUser === None)
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, adminHeaders, SC_OK)
+ }
+
jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
assert(res.owner === "alice")
+assert(res.proxyUser === Some("alice"))
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/?doAs=alice", Map(), headers = adminHeaders) {
res =>
+assert(res.owner === ADMIN)
+assert(res.proxyUser === Some("alice"))
assert(res.logs === IndexedSeq("log"))
delete(res.id, aliceHeaders, SC_OK)
}
}
it("should allow other users to see non-sensitive information") {
+ jpost[MockSessionView]("/", Map()) { res =>
+jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
+ assert(res.owner === null)
+ assert(res.proxyUser === None)
+ assert(res.logs === IndexedSeq("log"))
+}
+delete(res.id, adminHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
+jget[MockSessionView](s"/${res.id}") { res =>
+ assert(res.owner === "alice")
+ assert(res.proxyUser === Some("alice"))
+ assert(res.logs === IndexedSeq("log"))
+}
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
assert(res.owner === "alice")
+ assert(res.proxyUser === Some("alice"))
+ assert(res.logs === IndexedSeq("log"))
+}
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/?doAs=alice", Map(), headers = adminHeaders) {
res =>
+jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
+ assert(res.owner === ADMIN)
+ assert(res.proxyUser === Some("alice"))
assert(res.logs === IndexedSeq("log"))
Review comment:
Is this right? Isn't the user here "bob", which is neither an admin nor the
impersonated user?
Maybe name masking is confusing the compiler? (Two nested `res` variables.)
Also, separately, I couldn't find a test for this scenario:
- session for "alice" is created
- a request authenticated as an admin but with "doAs=bob" tries to do
something bob is not allowed to do with alice's session.
If that exists and I missed it's ok, but I couldn't find it after a couple
of look arounds.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251675173
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala
##
@@ -55,12 +54,11 @@ object BatchSession extends Logging {
id: Int,
request: CreateBatchRequest,
livyConf: LivyConf,
- accessManager: AccessManager,
owner: String,
+ impersonatedUser: Option[String],
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None): BatchSession = {
val appTag = s"livy-batch-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
Review comment:
It's just moving the check to the servlet, which happens before this is
called. Are you worried that another call site might be added and we forget
about the access check?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r250405695
##
File path:
server/src/test/scala/org/apache/livy/server/AccessManagerSuite.scala
##
@@ -116,4 +121,60 @@ class AccessManagerSuite extends FunSuite with Matchers
with LivyBaseUnitTestSui
accessManager.isUserAllowed("anyUser") should be (false)
}
+
+ test("check no impersonation") {
+val conf = new LivyConf()
+val accessManager = new AccessManager(conf)
+
+val req = mock[HttpServletRequest]
+when(req.getRemoteUser).thenReturn("abc")
+when(req.getParameter("doAs")).thenReturn("def")
+
+accessManager.getRequestUser(req) should be ("abc")
+accessManager.getImpersonatedUser(req) should be (None)
+accessManager.getEffectiveUser(req) should be ("abc")
+ }
+
+ test("check impersonation denied") {
+val conf = new LivyConf()
+ .set(LivyConf.IMPERSONATION_ENABLED, true)
+val accessManager = new AccessManager(conf)
+
+val req = mock[HttpServletRequest]
+when(req.getRemoteUser).thenReturn("abc")
+when(req.getParameter("doAs")).thenReturn("def")
+
+accessManager.getRequestUser(req) should be("abc")
+try {
+ accessManager.getImpersonatedUser(req) should be(Some("def"))
Review comment:
the "should be" doesn't seem necessary since you expect an exception here.
Also in the next test.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r249962561
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -97,47 +99,71 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
*/
def isAccessControlOn: Boolean = aclsOn
+ def getRequestUser(request: HttpServletRequest): String = {
+request.getRemoteUser
+ }
+
+ def getImpersonatedUser(request: HttpServletRequest): Option[String] = {
+val impersonatedUser = Option(request.getParameter("doAs"))
+impersonatedUser.filter(checkImpersonation(request, _))
+ }
+
+ def getEffectiveUser(request: HttpServletRequest): String = {
+val requestUser = getRequestUser(request)
+val impersonatedUser = getImpersonatedUser(request)
+impersonatedUser.getOrElse(requestUser)
+ }
+
/**
* Checks that the requesting user can impersonate the target user.
* If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
*/
- def checkImpersonation(
- target: Option[String],
- requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
-throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ def checkImpersonation(request: HttpServletRequest, impersonatedUser:
String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(request, impersonatedUser) || checkProxyUser(request,
impersonatedUser)) {
+return true
}
- target.orElse(Option(requestUser))
-} else {
- None
+ val requestUser = getRequestUser(request)
+ throw new AccessControlException(
+s"User '$requestUser' not allowed to impersonate '$impersonatedUser'.")
}
+false
+ }
+
+ def checkProxyUser(request: HttpServletRequest, impersonatedUser: String):
Boolean = {
+val proxyUser = getRequestUser(request)
+val remoteHost = request.getRemoteHost
+val allowedHosts = conf.hadoopConf.get("hadoop.proxyuser." + proxyUser +
".hosts")
Review comment:
It doesn't feel right to look at Hadoop's configuration when deciding
whether impersonation is allowed in Livy. They'll probably end up being
different.
e.g. Hadoop's configuration could allow `livy` to impersonate anybody and
that's it. But Livy could allow `admin` to impersonate other people when
performing Livy operations.
IIRC the current Livy behavior is "superusers can impersonate". What are you
trying to achieve here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r249964784 ## File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala ## @@ -158,7 +158,21 @@ abstract class SessionServlet[S <: Session, R <: RecoveryMetadata]( /** * Returns the remote user for the given request. Separate method so that tests can override it. */ - protected def remoteUser(req: HttpServletRequest): String = req.getRemoteUser() + protected def getOwner(req: HttpServletRequest): String = accessManager.getRequestUser(req) + + protected def checkBodyProxyUser(owner: String, bodyProxyUser: Option[String], Review comment: One parameter per line in multi-line declarations. Also it's not clear from the name what this method is doing. What is "body proxy user"? Or "proxyUser body" mentioned below? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
