[rzo1]

DRPC now checks nimbus.impersonation.authorizer for requests made on behalf of another user, as Nimbus and the UI already do. The HTTP request context now carries the remote address, so host lists in nimbus.impersonation.acl apply there too. Behaviour without an impersonation authorizer is unchanged.

Behaviour notes:

• A request with doAsUser but no authenticated user (no drpc.http.filter / ui.filter) is now denied with 401 by DRPC and the UI, whether or not an impersonation authorizer is configured. Before, DRPC let it act as the named user when no impersonation authorizer was set, and the UI failed with a 500.
• ImpersonationAuthorizer now logs "Allowing impersonation" at DEBUG instead of INFO (Nimbus, UI and DRPC). The "attempting to impersonate" INFO line remains.
• Behind a reverse proxy, host lists in nimbus.impersonation.acl match the proxy's address (documented in SECURITY.md).
• A topology client that impersonates while polling fetchRequest gets one INFO line per poll from the impersonation authorizer.

[jnioche]

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.

[rzo1]

Thanks @jnioche! All points are addressed in c7bfb06:

1. Unauthenticated doAsUser: SingleUserPrincipal.equals/hashCode are null-safe, so there is no NPE anymore. The request still counts as impersonation: without an impersonation authorizer DRPC behaves as before, with one it is denied with 401 (ImpersonationAuthorizer denies a real principal without a name). The UI had the same NPE through AuthorizedUserFilter; it now answers such requests with 401. Tests cover HTTP, the Thrift context and the authorizer.
2. Added the reverse proxy note to SECURITY.md.
3. isAllowedToImpersonateFromHost checks getHostAddress() first, so IP-based ACLs skip the reverse lookup (test with mocked InetAddress).

Nits: an allowed impersonated request now logs one INFO line, and the denial names the function.

Could you have another look?

[jnioche]

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.

[GGraziadei]

Hi @rzo1 thanks!
Just two non-blocking points.

[GGraziadei]

Nit: the comment is true today but getByName would silently start doing a DNS lookup per request if that ever changes. Instead InetAddress.ofLiteral(remoteAddr) gives the no-lookup guarantee explicitly (it throws IllegalArgumentException for anything that is not a literal).

Verified with java doc: https://cr.openjdk.org/~prr/8357176/api/java.base/java/net/InetAddress.html#ofLiteral(java.lang.String)

[GGraziadei]

Minor, no need to block on it. This check only looks at the request context, but for needsTopoId() endpoints it runs after the getTopologyConf call to Nimbus above, which it does not need. So a request that is going to be refused with 401 anyway first costs a Thrift round trip to Nimbus and leaves a getTopologyConf line in the Nimbus access log. Moving this if block above the needsTopoId() lookup would reject such requests right away, with no Nimbus call.