HyukjinKwon commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r4004067345
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/config/package.scala:
##########
@@ -294,6 +294,29 @@ 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, if it wraps " +
Review Comment:
Fixed in c4b3ba0. Reworded so the requirement is that the downstream filter
*rejects* unauthenticated requests, not merely wraps them.
`config/package.scala`, `docs/running-on-yarn.md:265`, and the second paragraph
of "What changes were proposed in this pull request?" now say: 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. The closing "Only set it to `false` ... together with
such an authentication filter" line now spells out "that rejects
unauthenticated requests".
##########
core/src/main/scala/org/apache/spark/SecurityManager.scala:
##########
@@ -409,6 +409,12 @@ private[spark] class SecurityManager(
aclUsers.contains(user) ||
aclGroups.contains(WILDCARD_ACL)) {
true
+ } else if (aclGroups.isEmpty) {
Review Comment:
Kept the short-circuit and pinned it in c4b3ba0. It is what keeps a denied
request from shelling out to `id -Gn` and logging an ERROR for the non-existent
sentinel -- in the default view-ACL config, and for every proxied request in
cluster mode. Added your `CountingGroupMappingServiceProvider` and
`SecurityManagerSuite` "SPARK-59312: no group ACLs means no group lookup",
which asserts the provider is never called; it passes with the branch and fails
without it (verified). The "How was this patch tested?" section now names
`SecurityManagerSuite` too.
--
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]
