HyukjinKwon commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r4001014736
##########
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:
Good catch. Switched to a non-empty sentinel
`__spark_untrusted_proxy_user__` and dropped the "match any ACL entry" clause
(9e7538e). Added `SPARK-59312: the untrusted proxy sentinel is denied by the AM
UI ACLs`, asserting a `SecurityManager` with `spark.acls.enable=true` denies it
for both view and modify.
##########
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:
Done in 9e7538e. The warning now branches on `spark.acls.enable`: with ACLs
on it states the denial, with ACLs off it states the option has no effect.
Added the "while ACLs are enabled" qualifier to the class comment and the
description too.
##########
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:
Applied the `isUserInACL` short-circuit in 9e7538e: when `aclGroups.isEmpty`
it returns false without the group lookup, so the default no longer shells out
to `id -Gn` (or logs the ERROR) on a denied request. With group ACLs configured
the sentinel would still shell out, as you noted; happy to add the full "cannot
exist" short-circuit as a follow-up if you prefer.
##########
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:
Moved above the `/* Client-mode AM configuration. */` comment, next to
`AM_FINAL_MSG_LIMIT`, with a note that it applies in cluster mode too. 9e7538e.
--
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]
