peter-toth commented on code in PR #58599: URL: https://github.com/apache/spark/pull/58599#discussion_r3980000765
########## docs/running-on-yarn.md: ########## @@ -248,6 +248,27 @@ To use a custom metrics.properties for the application master and executors, upd </td> <td>1.3.0</td> </tr> +<tr> + <td><code>spark.yarn.am.trustProxyUserCookie</code></td> + <td><code>true</code></td> + <td> + When <code>true</code> (default), the AM UI filter uses the <code>proxy-user</code> cookie set + by the YARN RM web proxy to determine the user for the AM UI view/modify ACLs. This forwarded + cookie is not cryptographically signed; fully guaranteeing its integrity would require the YARN + RM web proxy to sign it (a Hadoop-side change), so this option is an interim workaround until + then. The AM always installs its own filter first, so this option is meant for client mode, + where a separate authentication filter set in <code>spark.ui.filters</code> runs after it: the + last request wrapper in the chain determines the user, so that authentication filter already Review Comment: **Finding 1.** This answers the question at [issuecomment-5584138666](https://github.com/apache/spark/pull/58599#issuecomment-5584138666) by conceding the option does nothing. If the authentication filter "already establishes the request's user regardless of this option", then in the one setup the doc endorses, `false` changes no behaviour. That reads as an argument against merging rather than for it. The premise is also too strong. "The last request wrapper in the chain determines the user" only holds when the later filter always wraps. A filter that validates and then calls `chain.doFilter(req, resp)` unchanged leaves `AmIpServletRequestWrapper` outermost, so `getRemoteUser()` is still the cookie value. Three shapes do that: an IP or network allowlist, a token/header check that only accepts or rejects, and an SSO filter that passes whitelisted paths (health, metrics) straight through. That is the deployment where `false` is worth setting, and the doc never mentions it. Suggest replacing the "regardless of this option" clause with it, roughly: > An authentication filter that wraps the request overrides the cookie principal on its own, so this option matters for filters that authenticate without wrapping. There the AM would otherwise hand `HttpSecurityFilter` the unverified cookie user. ########## resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala: ########## @@ -695,7 +695,14 @@ 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) + // Only pass the init parameter when the cookie is not trusted; when trusted (the default) the + // filter parameters stay unchanged from the original behavior. + val params = if (sparkConf.get(AM_TRUST_PROXY_USER_COOKIE)) { Review Comment: **Finding 2.** The doc says "Only set it to `false` in client mode together with such an authentication filter", but nothing checks that. This method has both facts already: `driver.isEmpty` is exactly cluster mode, and `sparkConf.get(UI_FILTERS)` still holds the user's filters here (line 711 replaces them right after). The failure mode is an AM UI where every request passes both the view and the modify ACL, so silence seems too quiet. Rejecting the request, as proposed at [issuecomment-5581070262](https://github.com/apache/spark/pull/58599#issuecomment-5581070262), is one answer. A cheaper one is to refuse to arm it silently: ```scala val params = if (sparkConf.get(AM_TRUST_PROXY_USER_COOKIE)) { baseParams } else { if (driver.isEmpty || sparkConf.get(UI_FILTERS).isEmpty) { logWarning(s"${AM_TRUST_PROXY_USER_COOKIE.key} is false and no other UI filter will run, " + "so proxied requests carry no user and pass every view and modify ACL check.") } baseParams + (AmIpFilter.TRUST_PROXY_USER_PARAM -> "false") } ``` ########## docs/running-on-yarn.md: ########## @@ -248,6 +248,27 @@ To use a custom metrics.properties for the application master and executors, upd </td> <td>1.3.0</td> </tr> +<tr> + <td><code>spark.yarn.am.trustProxyUserCookie</code></td> + <td><code>true</code></td> + <td> + When <code>true</code> (default), the AM UI filter uses the <code>proxy-user</code> cookie set + by the YARN RM web proxy to determine the user for the AM UI view/modify ACLs. This forwarded + cookie is not cryptographically signed; fully guaranteeing its integrity would require the YARN + RM web proxy to sign it (a Hadoop-side change), so this option is an interim workaround until + then. The AM always installs its own filter first, so this option is meant for client mode, + where a separate authentication filter set in <code>spark.ui.filters</code> runs after it: the + last request wrapper in the chain determines the user, so that authentication filter already + establishes the request's user regardless of this option, and setting this to + <code>false</code> additionally makes the AM ignore the forwarded cookie. If this is set to + <code>false</code> without such an authentication filter (for example in cluster mode, where + the AM sets the UI filters and no filter can precede it), proxy requests are treated as having Review Comment: **Finding 3.** "no filter can precede it" understates cluster mode, and the gap matters to a reader deciding what to do. `ApplicationMaster.scala:711` does `System.setProperty(UI_FILTERS.key, amFilter)`, which replaces `spark.ui.filters` outright. `ApplicationMaster.main` has already pushed the properties file into `sys.props`, and `startUserApplication()` runs afterwards, so the user application's `new SparkConf()` sees `AmIpFilter` alone. So no other filter runs in cluster mode at all, before or after. As written, an operator could read this as "put my auth filter later in the list and cluster mode is fine". It would be dropped. Same point as [issuecomment-5584138666](https://github.com/apache/spark/pull/58599#issuecomment-5584138666). Suggest: "for example in cluster mode, where the AM replaces `spark.ui.filters` with its own filter, so no other filter runs". ########## resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala: ########## @@ -695,7 +695,14 @@ 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) + // Only pass the init parameter when the cookie is not trusted; when trusted (the default) the + // filter parameters stay unchanged from the original behavior. + val params = if (sparkConf.get(AM_TRUST_PROXY_USER_COOKIE)) { + baseParams + } else { + baseParams + (AmIpFilter.TRUST_PROXY_USER_PARAM -> "false") Review Comment: **Finding 4.** This mapping is the new user-facing wiring, and it has no coverage. The new `AmIpFilterSuite` case builds the init parameter by hand, so inverting this condition, or emitting the wrong parameter name, would keep the whole suite green. `addAmIpFilter` is awkward to call from a suite because of `client` and `driver`, but the interesting part is pure. Lifting it to the companion makes it testable the same way `ApplicationMasterSuite` already tests `getHistoryServerAddress`: ```scala private[yarn] def amIpFilterParams( baseParams: Map[String, String], trustProxyUserCookie: Boolean): Map[String, String] = { if (trustProxyUserCookie) baseParams else baseParams + (AmIpFilter.TRUST_PROXY_USER_PARAM -> "false") } ``` -- 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]
