fix: use short name from authToLocal rules as Kerberos authenticated identity#19434
fix: use short name from authToLocal rules as Kerberos authenticated identity#19434JWuCines wants to merge 1 commit into
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
This is an automated review by Codex GPT-5
f356ce0 to
ab60c25
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 changed files for correctness, edge cases, security, and integration risks; no issues found.
This is an automated review by Codex GPT-5
gianm
left a comment
There was a problem hiding this comment.
Given that the current logic has been in place for so long, and people have built their authorizer rules around it, the new behavior should be opt-in. I suggest a property like useShortNameIdentity, which would be specified as druid.auth.authenticator.<name>.useShortNameIdentity = true. The default should be false so existing behavior isn't changed.
Another thing to look at: the KerberosEscalator createEscalatedAuthenticationResult method still unconditionally uses the internalClientPrincipal. When useShortNameIdentity is set this should possibly also use the short name. Otherwise people will need to create authorization rules for the long name if it's used in an escalator.
With regard to docs, as long as you're updating those, it would be good to also clarify the meaning of DEFAULT for authToLocal. It's the default behavior but our docs don't make it clear what it means. I gather from Hadoop docs that DEFAULT strips the realm from the principal if it is the system's default Kerberos realm, and leaves the principal intact otherwise.
Finally, an embedded test would be useful to validate that everything works end to end. You could look at BasicAuthLdapConfigurationTest for an example of a test that uses an OpenLDAP server in a Docker container to test LDAP authentication. Something similar might work with a Kerberos server.
Description
The Kerberos authenticator was using
token.getName()(the full Kerberos principal, e.g.,user@EXAMPLE.COM) instead oftoken.getUserName()(the short name, e.g.,user) when constructing theAuthenticationResultidentity. This caused the authenticated identity to bypassauthToLocalname mapping rules, resulting in users being unable to authenticate when authorizer rules reference short names.Fixed the identity used in AuthenticationResult
Changed
KerberosAuthenticatorto usetoken.getUserName()instead oftoken.getName()when setting theAuthenticationResultidentity. This is consistent with:getRemoteUser()already returnstoken.getUserName()in the same filterDruidKerberosAuthenticationHandlercreates the token withnew AuthenticationToken(userName, clientPrincipal, getType())whereuserNameis the result ofKerberosName.getShortName()(i.e., theauthToLocal-mapped name)authToLocalconfiguration property, which exists to map Kerberos principals to local user names for authorizationAdded unit tests
Added
KerberosAuthenticatorFilterTest(4 tests) andDruidKerberosAuthenticationHandlerTokenTest(6 tests) to verify that:AuthenticationToken.getUserName()returns the short name andgetName()returns the full principalAuthenticationResultidentity uses the short name across simple principals, service principals, and customauthToLocalrulesUpdated documentation
Added a "User identity and
authToLocalrules" section to the Kerberos extension documentation explaining that the short name is used for authorization, with an upgrade note for users who may have authorizer rules referencing full Kerberos principals.Release note
The Kerberos authenticator now correctly uses the short name derived from
authToLocalrules as the authenticated user identity, instead of the full Kerberos principal. If you have authorizer rules that reference full Kerberos principals (e.g.,user@EXAMPLE.COM), update them to use the short name (e.g.,user) after upgrading.Key changed/added classes in this PR
KerberosAuthenticatorKerberosAuthenticatorFilterTestDruidKerberosAuthenticationHandlerTokenTestThis PR has: