peter-toth commented on code in PR #58599:
URL: https://github.com/apache/spark/pull/58599#discussion_r4003691506
##########
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:
**Finding 12.** "if it wraps the request, supplies the real user and
overrides the sentinel" makes wrapping the sufficient condition. It is not. A
wrapper can hand down a *null* user, and then `HttpSecurityFilter` reads
`getRemoteUser() == null` and `isUserInACL` returns `true` before it looks at
anything else. The parenthetical three lines up closes exactly that hole. A
downstream wrapper reopens it one filter later.
Hadoop's `AuthenticationFilter` is the concrete case, and it is the filter
an operator on YARN is most likely to reach for. With `simple` auth and
`simple.anonymous.allowed=true`, `PseudoAuthenticationHandler.authenticate`
returns `AuthenticationToken.ANONYMOUS`, whose `userName` is null
(`AuthToken`'s no-arg constructor). The filter wraps anyway:
```java
final AuthenticationToken authToken = token;
httpRequest = new HttpServletRequestWrapper(httpRequest) {
@Override
public String getRemoteUser() {
return authToken.getUserName(); // null for ANONYMOUS
}
...
```
`AmIpFilter` runs first (`YarnSchedulerBackend.scala:244`), so that wrapper
is the outer one and the sentinel is gone. With `spark.acls.enable=true`, this
option off, and that filter in `spark.ui.filters`, a proxied request is allowed
with no user. The doc says it is denied. `checkUIViewPermissions(null) = true`
is measured at
[r3987187316](https://github.com/apache/spark/pull/58599#discussion_r3987187316).
This is not a regression. With the cookie trusted the same request is
allowed too. What does not hold is the stated guarantee, in the one
configuration the doc recommends. The requirement on the downstream filter is
that it *rejects* unauthenticated requests, not that it wraps them. Suggest
saying that, roughly:
> ... where a separate authentication filter set in spark.ui.filters runs
after it and supplies the real user. That filter must reject unauthenticated
requests. One that wraps the request but leaves the user null (Hadoop's
AuthenticationFilter does this for anonymous requests) replaces the sentinel
with a null user, and a null user passes every view and modify ACL check.
`docs/running-on-yarn.md:265` carries the same sentence, and so does the
second paragraph of "What changes were proposed in this pull request?".
##########
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:
**Finding 13.** The branch changes no answer. With `aclGroups` empty the old
path also returned false, because `exists` on an empty set is false. So the
existing cases cover the outcome, and nothing covers what the branch is
actually for, which is not calling the group provider.
Measured: I deleted the branch on `9e7538ec` and re-ran both suites.
`yarn/testOnly *AmIpFilterSuite *ApplicationMasterSuite` stayed 8/8 and
`core/testOnly *SecurityManagerSuite` stayed 27/27. The new `AmIpFilterSuite`
sentinel case passes either way, since it only asserts the denial.
`SecurityManagerSuite` already has the provider plumbing next to
`DummyGroupMappingServiceProvider`. A counting provider pins it. This one
passes on `9e7538ec` and fails with the branch deleted, both measured:
```scala
class CountingGroupMappingServiceProvider extends
GroupMappingServiceProvider {
override def getGroups(username: String): Set[String] = {
CountingGroupMappingServiceProvider.calls += 1
Set[String]("group1")
}
}
object CountingGroupMappingServiceProvider {
var calls = 0
}
test("SPARK-59312: no group ACLs means no group lookup") {
CountingGroupMappingServiceProvider.calls = 0
val conf = new SparkConf()
.set(ACLS_ENABLE, true)
.set(UI_VIEW_ACLS, Seq("alice"))
.set(USER_GROUPS_MAPPING,
classOf[CountingGroupMappingServiceProvider].getName)
val securityManager = new SecurityManager(conf)
assert(!securityManager.checkUIViewPermissions("bob"))
assert(CountingGroupMappingServiceProvider.calls === 0)
}
```
The "How was this patch tested?" section does not mention this file either.
--
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]
