Fix UnknownError parsing Kerberos principal with escaped '@' - #51568
Open
ousamabenyounes wants to merge 1 commit into
Open
Fix UnknownError parsing Kerberos principal with escaped '@'#51568ousamabenyounes wants to merge 1 commit into
ousamabenyounes wants to merge 1 commit into
Conversation
KerberosPrincipal split the principal on every '@' and required exactly two parts, so a principal whose name component contains an escaped '\@' (valid per RFC 1964 section 2.1.1, e.g. ssiemel\@a.com@KEYCLOAK.ORG) was rejected and surfaced as an "UnknownError" during LDAP user sync. Parse the realm as the part after the single unescaped '@' instead, keeping the existing rejection of ambiguous (multiple unescaped '@') and realm-less principals. Closes keycloak#29316 Signed-off-by: Ben Younes <2910651+ousamabenyounes@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Kerberos principal parsing to support escaped @ characters while rejecting ambiguous separators.
Changes:
- Adds escape-aware realm separator detection.
- Adds unit tests for valid, escaped, and invalid principals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
KerberosPrincipal.java |
Implements escape-aware principal parsing. |
KerberosPrincipalTest.java |
Covers parsing behavior and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
KerberosPrincipal(String)split the incoming principal on every@and required exactly two parts:A Kerberos principal may legally contain an escaped
@(written\@) inside its name component — see RFC 1964, section 2.1.1. For such a principal, e.g.ssiemel\@a.com@KEYCLOAK.ORG,split("@")yields three parts and the constructor throwsIllegalArgumentException. On the LDAP user-sync path (LDAPStorageProvider→new KerberosPrincipal(...)) this is not handled gracefully and surfaces asCould not sync users: 'UnknownError'in the admin console (with nothing in the server log), exactly as reported in the issue.Fix
Parse the realm as the part after the single unescaped
@, treating an escaped\@inside the name as a literal (escape detection via backslash parity). Behaviour is otherwise unchanged:john@KEYCLOAK.ORGjohn, realmKEYCLOAK.ORG(unchanged)ssiemel\@a.com@KEYCLOAK.ORGssiemel\@a.com, realmKEYCLOAK.ORG(fixed — previously threw)a@b@KEYCLOAK.ORG(multiple unescaped@)john/john@(no realm / empty realm)All four production callers (
LDAPStorageProvider,KerberosFederationProvider,SPNEGOAuthenticator,KerberosUsernamePasswordAuthenticator) route through this one constructor, so fixing the shared parser repairs every path, including the reported LDAP-sync reproducer. The change is intentionally limited toKerberosPrincipaland preserves the original exception message.Test verification (RED → GREEN)
New unit test
KerberosPrincipalTest(plain JUnit, no running server required).RED — on unmodified
main, before the production change:GREEN — with the fix:
./mvnw -pl federation/kerberos spotless:checkand the module's full test suite (KerberosFederationProviderFactoryTest+KerberosPrincipalTest, 10 tests) also pass.Note on AI usage
AI agents were used to assist with investigating and implementing this change. I understand every line of the change and can explain and revise it.
Closes #29316