peter-toth commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r4004932046
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala:
##########
@@ -695,7 +695,27 @@ 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. The effect
+ // depends on whether the AM UI ACLs are enabled: with them off (the
default) the option has no
+ // effect, since a request is allowed before its user is even looked at.
+ if (!trustProxyUserCookie) {
+ if (sparkConf.get(ACLS_ENABLE)) {
+ logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)}
is false, so " +
Review Comment:
**Finding 14.** In cluster mode this warning's escape clause cannot be
taken, and this method can tell.
`driver.isEmpty` is exactly cluster mode: `runDriver` at `:497` is the only
call site that passes `None`, while `:307` (unmanaged AM) and `:554`
(`runExecutorLauncher`) both pass `Some(driverRef)`. On the `None` path, `:724`
does `System.setProperty(UI_FILTERS.key, amFilter)`, which replaces
`spark.ui.filters` wholesale, and `runDriver` runs it before
`startUserApplication()`. So no other filter runs, and none can establish the
user. The config doc states this itself at `config/package.scala:315-316`.
An operator reading this line in a cluster-mode AM log is told they can fix
it with an authentication filter. They cannot - it is silently dropped. And
this is the case where they have just made the AM UI inaccessible to everyone,
which is the part worth saying.
This is not my round-2 finding 6 coming back. There the argument was that no
conf inspection can tell whether a filter *establishes a user*, so
`UI_FILTERS.isEmpty` could never be accurate. Cluster mode is a different fact:
no filter runs at all, and `driver` already carries it.
```scala
if (!trustProxyUserCookie) {
if (!sparkConf.get(ACLS_ENABLE)) {
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.")
} else if (driver.isEmpty) {
logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false in cluster " +
log"mode, where the AM replaces ${MDC(LogKeys.CONFIG2,
UI_FILTERS.key)} with its own " +
log"filter, so no other filter can establish the user: every proxied
request is denied " +
log"by the AM UI view and modify ACLs.")
} else {
logWarning(log"${MDC(LogKeys.CONFIG, AM_TRUST_PROXY_USER_COOKIE.key)} is
false, so " +
log"proxied requests are treated as an unauthenticated user and are
denied by the AM " +
log"UI view and modify ACLs unless another authentication filter
establishes the user.")
}
}
```
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config/package.scala:
##########
@@ -294,6 +294,32 @@ package object config extends Logging {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("1m")
+ // Not under the client-mode section below: this takes effect in cluster
mode too, and that is
+ // where its effect is largest (the AM replaces spark.ui.filters with its
own filter there).
+ 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. When false, the AM UI filter does not trust
the cookie and fails " +
+ "closed: it treats a proxied request as an unauthenticated user that
is in no ACL, so " +
+ "while AM UI ACLs are enabled (spark.acls.enable=true) the request is
denied unless " +
+ "another authentication filter establishes the user. (Leaving the
request with no user " +
+ "instead would not help: a null user passes every view and modify ACL
check.) 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
and supplies the " +
+ "real user. That filter must reject unauthenticated requests, not
merely wrap them: one " +
+ "that wraps the request but leaves the user null (as Hadoop's
AuthenticationFilter does " +
+ "for anonymous requests) replaces the sentinel with a null user, and a
null user passes " +
+ "every ACL check. In cluster mode the AM replaces spark.ui.filters
with its own filter, " +
+ "so no other filter runs and, with the cookie not trusted, all proxied
requests are " +
+ "denied while ACLs are enabled. Only set it to false in client mode
together with such " +
Review Comment:
**Finding 15.** This closing instruction names one of the two properties the
downstream filter needs. Rejecting unauthenticated requests is what keeps a
null user from replacing the sentinel, which is finding 12. It does not get an
authenticated request *past* the ACL check - for that the filter also has to
set the request's user. The sentence at `:311-312` says that ("supplies the
real user"); this line, the one an operator acts on, drops it.
Spark's own `JWSFilter` is the filter that separates the two, and
`docs/security.md:834-838` recommends it for `spark.ui.filters` on all UI
ports. It rejects unauthenticated requests with 403, then forwards the request
**unwrapped** - `core/src/main/scala/org/apache/spark/ui/JWSFilter.scala:70`
passes the original `req`, not a wrapper - so `getRemoteUser()` is still the
sentinel when `HttpSecurityFilter` runs. With `spark.acls.enable=true`,
`spark.ui.filters=org.apache.spark.ui.JWSFilter` and this option `false`, a
request carrying a valid token is denied. That configuration satisfies this
sentence as written and still leaves the AM UI inaccessible.
Read from the source, not run end to end: `JWSFilter` is package-private to
`org.apache.spark.ui`, so a chain test would have to live in `core` while
`AmIpFilter` is in `yarn`.
Suggest naming both properties, and saying which filters qualify:
```scala
"denied while ACLs are enabled. Only set it to false in client mode together
with an " +
"authentication filter that both rejects unauthenticated requests and sets
the request's " +
"user, such as Hadoop's AuthenticationFilter with a Kerberos handler.
Spark's own JWSFilter " +
"does not qualify: it rejects unauthenticated requests but forwards them
unwrapped, so the " +
"sentinel survives and the request is denied.")
```
`docs/running-on-yarn.md:270-272` carries the same sentence.
--
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]
