peter-toth commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r3987187326
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala:
##########
@@ -695,7 +695,20 @@ 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: when the cookie is not trusted and
no other UI filter
+ // will run to establish the user (cluster mode replaces spark.ui.filters
with the AM's own
+ // filter; client mode may also have no other filter), proxied requests
carry no user and pass
+ // every view and modify ACL check.
+ if (!trustProxyUserCookie && (driver.isEmpty ||
sparkConf.get(UI_FILTERS).isEmpty)) {
Review Comment:
**Finding 6.** The condition tests whether another UI filter is
*configured*. What matters is whether another filter *establishes a user*, and
the config text draws exactly that distinction ("an authentication filter that
wraps the request ... but a filter that authenticates without wrapping the
request ...").
An IP allowlist or a token check in `spark.ui.filters` makes
`sparkConf.get(UI_FILTERS).isEmpty` false, so the warning stays silent. That is
the configuration the doc recommends, and per finding 1 it is also the one
where every ACL check starts passing. So the warning fires only for the case
the doc already tells operators not to use, and is quiet for the case it tells
them to use.
No conf inspection can tell whether a filter wraps the request, so this
condition cannot be made accurate. Warn whenever the option is off, and state
the effect rather than the cause:
```scala
if (!trustProxyUserCookie) {
logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false, so proxied " +
log"requests carry no user and pass every view and modify ACL check.")
}
```
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config/package.scala:
##########
@@ -301,6 +301,32 @@ package object config extends Logging {
.intConf
.createWithDefault(1)
+ private[spark] val AM_TRUST_PROXY_USER_COOKIE =
+ ConfigBuilder("spark.yarn.am.trustProxyUserCookie")
+ .doc("When true (default), the YARN AM UI filter uses the 'proxy-user'
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
spark.ui.filters " +
+ "runs after it. The last request wrapper in the chain determines the
user: an " +
+ "authentication filter that wraps the request overrides the cookie
principal on its own, " +
+ "so this option changes nothing there; but a filter that authenticates
without wrapping " +
+ "the request (for example an IP or network allowlist, a token/header
check that only " +
+ "accepts or rejects, or an SSO filter that passes whitelisted paths
straight through) " +
+ "leaves the AM's own request wrapper outermost, so getRemoteUser() is
still the " +
+ "unverified cookie value. That is the case this option is for: setting
it to false makes " +
Review Comment:
**Finding 1.** This is the sentence I asked for in round 1, and my round-1
suggestion was wrong. I withdraw it. The non-wrapping-filter case is not where
the option helps. It is where the option removes the ACL check.
Once the cookie is ignored and nothing downstream wraps the request,
`HttpSecurityFilter` reads `getRemoteUser() == null`, and
`SecurityManager.isUserInACL`
(`core/src/main/scala/org/apache/spark/SecurityManager.scala:406`)
short-circuits to `true` on a null user:
```scala
if (user == null ||
!aclsEnabled() ||
aclUsers.contains(WILDCARD_ACL) ||
...) {
true
}
```
Measured against a `SecurityManager` built with `spark.acls.enable=true`,
`spark.ui.view.acls=alice`, `spark.modify.acls=alice`, `spark.admin.acls=admin`:
```
checkUIViewPermissions("mallory") = false // forged cookie user, denied
today
checkUIViewPermissions(null) = true // the same request with this
option off
checkModifyPermissions(null) = true // kill job / kill stage
checkAdminPermissions(null) = true // so ?doAs=<anyone> passes as
well
```
So for the three filter shapes this paragraph names, `false` does not stop a
forged user from being honoured. It stops every user from being checked.
`false` never denies anything that `true` allows, in any configuration. It
either changes nothing (`spark.acls.enable=false`, or the cookie user was in
the ACLs anyway) or it widens access.
That means the next paragraph, "If this is set to `false` without such an
authentication filter ... exposing the AM UI", describes the recommended
configuration too, not just the discouraged one.
Two ways out. The first is the alternative already offered in
[issuecomment-5581070262](https://github.com/apache/spark/pull/58599#issuecomment-5581070262):
- Fail closed. When the cookie is not trusted, `AmIpFilter` rejects the
proxied request instead of forwarding it anonymously. This also cuts off the
wrapping-auth-filter case, since the chain stops before that filter runs.
- Keep it anonymous, and say plainly that `false` turns the AM UI view and
modify ACLs off for proxied requests in every configuration where it has any
effect. The config then has to stand on that, not on "the request reaches the
ACL check with no forged user".
Making the ACL layer itself fail closed on an unknown user would be the real
fix, but that is a `SecurityManager` change well outside this PR.
The same sentence is in `docs/running-on-yarn.md:267`.
--
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]
