jnioche commented on PR #9183: URL: https://github.com/apache/storm/pull/9183#issuecomment-6335486664
AI assisted review Verdict: approve with one fix. The change copies what Nimbus already does in checkAuthorization: the impersonation check runs before the DRPC ACL, and if no authorizer is configured it logs a warning and allows the request. nimbus.impersonation.authorizer has no default in defaults.yaml, so deployments that don’t set it see no change. It closes a real gap: a Kerberos or SASL client can present an authorization ID that differs from its authentication ID, and DRPC never checked it. Should fix Unauthenticated HTTP request with doAsUser now throws an NPE and returns a 500. This happens when no drpc.http.filter is set. DefaultHttpCredentialsPlugin then sets realPrincipal = new SingleUserPrincipal(null). DRPC now calls isImpersonating() on every request, which calls SingleUserPrincipal.equals, which calls userName.equals(...) on a null name. This happens even without an impersonation authorizer, so it contradicts “behaviour unchanged”. Fix: have DefaultHttpCredentialsPlugin ignore or reject doAsUser when there’s no authenticated user, or make equals null-safe. Add a test for this case. Worth noting 2. Reverse proxies. getRemoteAddr() returns the proxy’s IP, so host lists in nimbus.impersonation.acl match the proxy, not the client. One line in SECURITY.md would cover it. 3. Reverse DNS on every request. Unless the ACL uses the wildcard, isAllowedToImpersonateFromHost calls getCanonicalHostName() and getHostName() before getHostAddress(). That now runs on every impersonated DRPC HTTP request. Checking getHostAddress() first would skip the lookup whenever the ACL lists IPs. Nits An allowed impersonated execute call logs three INFO lines: one from DRPC and two from ImpersonationAuthorizer. The denial message names the operation but not the DRPC function; adding the function would help debugging. The null remoteAddress guard in ImpersonationAuthorizer is good, and it also hardens the Nimbus and Supervisor paths. The 12 new tests cover deny, permit, host lists, unknown address and no-authorizer, for both the Thrift context and HTTP. The one missing case is unauthenticated doAsUser from item 1. -- 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]
