jnioche commented on PR #9183:
URL: https://github.com/apache/storm/pull/9183#issuecomment-6336553863
thanks @rzo1
**Verdict: approve.** The new commit addresses every point from my earlier
review. One design inconsistency remains, and I'd settle it before merging.
**Earlier points, now fixed**
- **The NPE:** `SingleUserPrincipal` equality is now null-safe, so an
unauthenticated request with `doAsUser` counts as impersonation instead of
throwing an NPE. When an impersonation authorizer is configured,
`ImpersonationAuthorizer` now denies it. It also checks the name before
`ptol.toLocal`, so `KerberosPrincipalToLocal` can't throw on a null name.
- **Reverse DNS:** the IP address is checked first, and a test confirms no
reverse lookup happens when the ACL lists IPs.
- **Logging:** one INFO line per allowed impersonated request instead of
three.
- **Error message and docs:** denials now name the DRPC function, and
`SECURITY.md` covers the reverse proxy case.
- **Bonus fix:** the UI no longer throws an NPE on
`remoteAddress.toString()` when the address is null.
**Should settle**
1. **DRPC and the UI disagree on unauthenticated `doAsUser`.**
- The UI now always rejects it with a 401.
- DRPC still allows it when no impersonation authorizer is set, which is
the default;
`testHttpUnauthenticatedImpersonationWithoutImpersonationAuthorizer` asserts
the 200.
- That keeps a bypass that predates this PR: if DRPC HTTP has no
`drpc.http.filter` but a DRPC ACL authorizer is configured, the ACL denies
anonymous callers, yet any anonymous caller can add `doAsUser=<allowed user>`
and get through.
- An unauthenticated request claiming an identity is never legitimate, so
I'd deny it in `DRPC.checkAuthorization` as the UI does, whether or not an
authorizer is set. Note that in the release notes rather than keeping the old
behaviour.
**Minor**
2. **Nimbus and UI logs change too.** Moving "Allowing impersonation" to
DEBUG in `ImpersonationAuthorizer` affects them as well, not just DRPC. The
"attempting to impersonate" INFO line still remains, but anyone grepping audit
logs for "Allowing" would lose it. Worth a line in the release notes.
--
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]
