RANGER-5746: Improvement in Ranger policy engine - #1156
Conversation
vyommani
left a comment
vyommani
left a comment
There was a problem hiding this comment.
This fix correctly mitigates the reported issue — folding with Character.toLowerCase(Character.toUpperCase(ch)) instead of a plain toLowerCase matches the equivalence classes equalsIgnoreCase actually uses, so case-variant resources (e.g. Turkish dotless i, long s) no longer get routed to a trie branch missing a DENY evaluator that the real matcher would have matched. Test coverage looks solid.
One thing worth flagging, not for this PR: there's a narrower, related gap for supplementary-plane (astral) characters — scripts like Deseret or Adlam encoded as surrogate pairs — where a sibling policy can force the trie to split mid-pair and reintroduce the same class of issue one level deeper. This is very unlikely in production (resource names are essentially always ASCII/BMP), and properly closing it needs more than a localized fix, so I'd rather track it in a follow-up JIRA than extend this PR.
Recommend merging as-is.
|
|
||
| private Character getLookupChar(char ch) { | ||
| return optIgnoreCase ? Character.toLowerCase(ch) : ch; | ||
| return optIgnoreCase ? Character.toLowerCase(Character.toUpperCase(ch)) : ch; |
There was a problem hiding this comment.
@maheshbandal15 - would using Character.toUpperCase(ch), instead of two level conversion - Character.toLowerCase(Character.toUpperCase(ch)), address the reported issue?
There was a problem hiding this comment.
sigle toUpperCase(ch) alone fixes this specific reported case (ı vs i), but not others like İ vs i, Kelvin sign vs k, or Ångström sign vs å, which still diverge under single toUpperCase. The double-fold (toLowerCase(toUpperCase(ch))) is needed to cover those too, since it matches equalsIgnoreCase's actual behavior rather than just this one character. Please run the attached java program for detail.
There was a problem hiding this comment.
Thank you @vyommani for the details and the program to demonstrate the issue.
@maheshbandal15 - can you please make sure that unit tests cover these cases?
4 participants
What changes were proposed in this pull request?
Align case-insensitive folding in RangerResourceTrie with the folding used by resource matchers.
When ignoreCase is enabled, the resource trie indexes child nodes using getLookupChar(), while policy resource matching uses StringUtils.equalsIgnoreCase() (equivalent to String.regionMatches(true, …)). Those two paths did not use the same Unicode case fold:
Trie lookup (before): Character.toLowerCase(ch)
Matchers: upper-case, then lower-case (equalsIgnoreCase semantics)
For some Unicode characters these folds diverge (for example i / ı, s / ſ, and other BMP equivalents). The trie could route a request to a different branch than the matcher would treat as equal, so candidate policy evaluators returned from trie pre-filtering could be incomplete relative to full matcher evaluation.
This change updates getLookupChar() to use Character.toLowerCase(Character.toUpperCase(ch)) when ignoreCase is true, matching the matcher’s ignore-case behavior. Case-sensitive resources are unchanged.
How was this patch tested?
New unit tests (TestRangerResourceTrieCaseFolding):
Manual verification: Built ranger locally using mvn clean install; all targeted and related tests pass.