dongjoon-hyun commented on PR #58599:
URL: https://github.com/apache/spark/pull/58599#issuecomment-5672098886
Thanks for the updates. The fail-closed path is correct now and CI is green.
Two remaining points, both about whether the option is worth shipping, plus a
few nits.
**1. Even with fail-closed, `false` is still a no-op in the one
configuration the doc recommends**
The doc says to set this only in client mode together with an authentication
filter that
rejects unauthenticated requests and sets the request's user. In that setup
`AmIpFilter` runs
first (`YarnSchedulerBackend.addWebUIFilter` prepends it), and the auth
filter's wrapper is the
outer one, so `getRemoteUser()` is the real user whether the inner principal
is the cookie user or
the sentinel. `true` and `false` produce the same result.
The only configuration where `false` changes anything is one with no
downstream filter that
sets a user (cluster mode, or `JWSFilter`-style reject-only filters), and
there it denies every
proxied request while ACLs are enabled. That is the same conclusion as your
earlier
"Closing after review" comment; the reopen didn't refute it. So what we
would be adding is
effectively "disable the AM UI through the proxy", and I'm not sure that
needs a dedicated config.
**2. The "unsigned cookie" premise is weaker than the description suggests**
I checked Hadoop 3.4.1 `WebAppProxyServlet.proxyLink`:
- the client's `Cookie` header is not in `PASS_THROUGH_HEADERS`, so it never
reaches the AM;
- the `proxy-user` cookie is set by the proxy itself from
`req.getRemoteUser()`, i.e. the RM's
authenticated user;
- `AmIpFilter` only reads the cookie for requests whose remote address is a
proxy host.
So forging it requires spoofing the proxy's IP or running on the proxy host.
And if the RM uses
`simple` authentication, the identity is already forgeable via the
`user.name` parameter, which
signing the cookie would not fix. Is there a concrete deployment where this
option closes a real
gap? Without one, I'd lean towards not adding it.
**Nits**
- Only one of the two new `AmIpFilterSuite` tests carries the `SPARK-59312:`
prefix.
- The same ~20-line explanation is repeated three times (`.doc()`,
`running-on-yarn.md`, and the
`AmIpFilter` class comment). The config doc could be much shorter, with
the detail kept in
`running-on-yarn.md`.
- The new comment in `SecurityManager.isUserInACL` refers to the YARN AM
sentinel; a `core`
comment probably shouldn't depend on a `yarn` concept. "No group ACLs, so
skip the group
lookup" is enough.
--
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]
