dongjoon-hyun commented on PR #58599:
URL: https://github.com/apache/spark/pull/58599#issuecomment-5581070262
Thanks for working on this. A few review comments.
**1. Setting this to `false` bypasses the ACLs rather than tightening them**
When `trustProxyUser` is false, `user` stays `null` and the request goes
down the
unwrapped path, so `getRemoteUser()` returns `null` for every proxied
request.
`SecurityManager.isUserInACL` returns `true` unconditionally for a null user:
```scala
if (user == null || !aclsEnabled() || ...) true
```
So on a cluster locked down with `spark.acls.enable=true` and
`spark.ui.view.acls=alice`, enabling this option lets *any* request through
the
proxy pass both the view ACL and the modify ACL (which includes killing
jobs).
No `doAs` trick is needed - a plain GET is enough.
The PR description frames this as operators who "prefer proxy requests to
carry
no user identity", and the config doc says it "disables per-user AM UI ACLs
through the proxy". That is literally true, but a reader will likely read it
as
a hardening switch when the effect is the opposite.
The one deployment where this does help is when a real authentication filter
(SPNEGO, Knox, ...) is installed earlier in `spark.ui.filters`, so that
`AmIpFilter` no longer overwrites the authenticated principal with the cookie
principal (`JettyUtils.addFilters` runs user filters in order, with
`HttpSecurityFilter` last). But that setup does not work in cluster mode
today:
`ApplicationMaster.addAmIpFilter` does
`System.setProperty(UI_FILTERS.key, amFilter)`, replacing `spark.ui.filters`
with `AmIpFilter` alone. (That part is pre-existing, not introduced here.)
Could you clarify which deployment this targets? Depending on the answer, I
think either:
- the config doc should state explicitly that, absent another authentication
filter, all proxied requests become anonymous and pass every view/modify
ACL
check; or
- the `false` path should reject the request instead of proceeding
anonymously.
**2. Missing entry in `docs/running-on-yarn.md`**
Most `spark.yarn.am.*` configs are documented there. Coverage is not perfect
(e.g. `spark.yarn.am.limitActiveProcessorCount.enabled` is missing), but a
security-relevant config in particular deserves a documented entry - and it
is a
good place for the warning above.
**3. Naming: config vs init parameter**
The config is `spark.yarn.am.trustProxyUserCookie` but the init parameter is
`TRUST_PROXY_USER`. Naming it `TRUST_PROXY_USER_COOKIE` would make the two
easier to connect.
**4. Nit: test placement**
`AmIpFilterSuite` is documented as "A port of
org.apache.hadoop.yarn.server.webproxy.amfilter.TestAmFilter". The new
Spark-specific test is inserted ahead of the ported ones; appending it at the
end would keep the ported block contiguous.
**5. Nit: the init parameter is always sent**
Adding it only when the value is `false` would leave the filter params and
system properties byte-for-byte unchanged on the default path.
--
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]
