peter-toth commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r3995717517
##########
resource-managers/yarn/src/main/java/org/apache/spark/deploy/yarn/AmIpFilter.java:
##########
@@ -62,6 +67,14 @@ public class AmIpFilter implements Filter {
private static final String RM_HA_URLS = "RM_HA_URLS";
// WebAppProxyServlet is defined in WebAppProxyServlet in the original
Hadoop code
public static final String PROXY_USER_COOKIE_NAME = "proxy-user";
+ // Spark addition: init parameter name controlling whether the proxy-user
cookie is trusted.
+ public static final String TRUST_PROXY_USER_PARAM =
"TRUST_PROXY_USER_COOKIE";
+ // Spark addition: the sentinel principal name used to fail closed when the
proxy-user cookie is
+ // not trusted. It is the empty string -- a non-null user that cannot be a
real principal or
+ // match any ACL entry -- so SecurityManager denies a request carrying it (a
null user, by
+ // contrast, is treated as allowed by every ACL check).
+ @VisibleForTesting
+ static final String UNTRUSTED_PROXY_USER = "";
Review Comment:
**Finding 7.** The comment above says the sentinel "cannot be a real
principal or match any ACL entry". The second half does not hold.
`SecurityManager` seeds both ACL sets from a property whose fallback is the
empty string
(`core/src/main/scala/org/apache/spark/SecurityManager.scala:77-82`):
```scala
private val defaultAclUsers = Set[String](System.getProperty("user.name",
""),
Utils.getCurrentUserName())
setViewAcls(defaultAclUsers, sparkConf.get(UI_VIEW_ACLS))
setModifyAcls(defaultAclUsers, sparkConf.get(MODIFY_ACLS))
```
An empty `user.name` puts `""` straight into `viewAcls` and `modifyAcls`,
and `isUserInACL` then returns `true` on `aclUsers.contains(user)`. Measured
with `spark.acls.enable=true` and `spark.ui.view.acls=alice`:
```
user.name set viewAcls = ptoth,alice checkUIViewPermissions("") =
false
user.name empty viewAcls = ,ptoth,alice checkUIViewPermissions("") =
true
```
`java -Duser.name= -XshowSettings:properties -version` prints `user.name =`,
so the empty value does survive to the JVM. It takes an explicit `-Duser.name=`
to get there, so this is not a live exploit. It is the one guarantee the whole
fail-closed path rests on, though, and when it breaks the sentinel passes every
view, modify and admin check. That is finding 1 back through another door, and
it fails silently.
A non-empty sentinel closes it by construction, and it reads better in the
403 body, which is currently `User is not authorized to access this page.`:
```suggestion
static final String UNTRUSTED_PROXY_USER =
"__spark_untrusted_proxy_user__";
```
The "or match any ACL entry" clause in the comment above should go with it.
Worth pinning as well, since nothing would catch a regression here. The
constant is visible from the yarn tests, so it can sit next to the new
`AmIpFilterSuite` case. It passes today; it is there to keep the invariant:
```scala
test("SPARK-59312: the untrusted proxy sentinel is denied by the AM UI
ACLs") {
val conf = new SparkConf().set(ACLS_ENABLE, true).set(UI_VIEW_ACLS,
Seq("alice"))
val sm = new SecurityManager(conf)
assert(!sm.checkUIViewPermissions(AmIpFilter.UNTRUSTED_PROXY_USER))
assert(!sm.checkModifyPermissions(AmIpFilter.UNTRUSTED_PROXY_USER))
}
```
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala:
##########
@@ -695,7 +695,22 @@ private[spark] class ApplicationMaster(
/** Add the Yarn IP filter that is required for properly securing the UI. */
private def addAmIpFilter(driver: Option[RpcEndpointRef], proxyBase: String)
= {
val amFilter = classOf[AmIpFilter].getName
- val params = client.getAmIpFilterParams(yarnConf, proxyBase)
+ val baseParams = client.getAmIpFilterParams(yarnConf, proxyBase)
+ val trustProxyUserCookie = sparkConf.get(AM_TRUST_PROXY_USER_COOKIE)
+ // Refuse to arm the option silently. Whether another filter establishes
the user cannot be
+ // told from the configuration -- an IP allowlist or token filter in
spark.ui.filters may run
+ // without wrapping the request -- so state the effect rather than guess
the cause: with the
+ // cookie not trusted, the AM filter fails closed and proxied requests are
treated as an
+ // unauthenticated user, so AM UI ACLs deny them unless another
authentication filter
+ // establishes the user.
+ if (!trustProxyUserCookie) {
+ logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false, so proxied " +
Review Comment:
**Finding 8.** `spark.acls.enable` defaults to `false`
(`core/src/main/scala/org/apache/spark/internal/config/UI.scala:219-222`), and
`isUserInACL` returns `true` on `!aclsEnabled()` before it looks at the user at
all. So in the default configuration this option denies nothing, and this
warning says it does. Measured with `spark.ui.view.acls=alice`:
```
spark.acls.enable=true checkUIViewPermissions("") = false
spark.acls.enable=false checkUIViewPermissions("") = true
```
The config doc and `running-on-yarn.md` both carry the "while ACLs are
enabled" qualifier. This warning is what an operator actually sees, and it is
their only signal that they did or did not get the hardening they asked for.
The method can tell the two cases apart, and `config.UI._` is already imported
here:
```scala
if (!trustProxyUserCookie) {
if (sparkConf.get(ACLS_ENABLE)) {
logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false, so proxied " +
log"requests are treated as an unauthenticated user and are denied by
the AM UI view " +
log"and modify ACLs unless another authentication filter establishes
the user.")
} else {
logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false but " +
log"${MDC(LogKeys.CONFIG2, ACLS_ENABLE.key)} is false, so it has no
effect. The AM UI " +
log"view and modify ACLs allow every user.")
}
}
```
The same qualifier is missing from the class comment at
`AmIpFilter.java:48-52` and from the "What changes were proposed" section of
the description. Both say the request is denied, with no mention that ACLs have
to be on.
##########
resource-managers/yarn/src/main/java/org/apache/spark/deploy/yarn/AmIpFilter.java:
##########
@@ -162,6 +183,16 @@ public void doFilter(ServletRequest req, ServletResponse
resp,
}
ProxyUtils.sendRedirect(httpReq, httpResp, redirect.toString());
+ } else if (!trustProxyUser) {
+ // Spark addition: the proxy-user cookie is not trusted. It is not
cryptographically
+ // verified, so rather than read it, fail closed: wrap the request with
a sentinel principal
+ // that is in no ACL, so a SecurityManager denies proxied requests that
reach the ACL check
+ // with it. Leaving the request with no user instead would pass every
view and modify ACL
+ // check, because a null user is treated as allowed. A downstream
authentication filter in
+ // spark.ui.filters that wraps the request replaces this principal with
the real user, so
+ // that case (the intended use, in client mode) is unaffected.
+ AmIpPrincipal principal = new AmIpPrincipal(UNTRUSTED_PROXY_USER);
Review Comment:
**Finding 9.** When the sentinel is denied, it is denied the slow way.
`isUserInACL` falls past `aclUsers.contains(user)` into
`Utils.getCurrentUserGroups(sparkConf, "")`, which builds a fresh
`ShellBasedGroupsMappingProvider` on every call. Observed while measuring
finding 7:
```
ERROR Utils: Error getting groups for user=
org.apache.spark.SparkException: Process List(/usr/bin/id, -Gn, ) exited
with code 1
at org.apache.spark.util.Utils$.executeAndGetOutput(Utils.scala:1226)
at
org.apache.spark.security.ShellBasedGroupsMappingProvider.getUnixGroups(ShellBasedGroupsMappingProvider.scala:44)
```
That is two forks per request, since `idPath` is a `lazy val` on the fresh
instance so `which id` runs first, plus an ERROR stack trace. In cluster mode
with the option on, every proxied request takes this path, static assets
included, and an unauthenticated client drives it. A non-empty sentinel does
not help: `id -Gn __spark_untrusted_proxy_user__` fails the same way.
The cheap fix is in `isUserInACL`, which has nothing to look up when no
group ACLs are configured:
```scala
} else if (aclGroups.isEmpty) {
false
} else {
val userGroups = Utils.getCurrentUserGroups(sparkConf, user)
logDebug(s"user $user is in groups ${userGroups.mkString(",")}")
aclGroups.exists(userGroups.contains(_))
}
```
That covers the default. With group ACLs configured the sentinel still
shells out, so a `SecurityManager` short-circuit on a user that cannot exist is
the complete answer. Fine as a follow-up if you would rather keep this PR
inside the yarn module.
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config/package.scala:
##########
@@ -301,6 +301,27 @@ package object config extends Logging {
.intConf
.createWithDefault(1)
+ private[spark] val AM_TRUST_PROXY_USER_COOKIE =
Review Comment:
**Finding 10.** `/* Client-mode AM configuration. */` at `:297` groups the
configs that only apply in client mode, like `spark.yarn.am.cores` right above
it ("Number of cores to use for the YARN Application Master in client mode").
This one applies in cluster mode too, and cluster mode is where its effect is
largest, since your own doc says every proxied request is denied there. Moving
it above the section comment, next to `AM_FINAL_MSG_LIMIT` at `:290`, keeps the
grouping honest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
