-
Notifications
You must be signed in to change notification settings - Fork 1k
RANGER-5215 : Policy authorisation fails for Ranger Plugins in case of users/groups converted by Ranger userysnc as per given Regex #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerDefaultRequestProcessor.java
Outdated
Show resolved
Hide resolved
ugsync-util/src/test/java/org/apache/ranger/ugsynutil/transform/TestRegEx.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/model/UgsyncNameTransformRules.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java
Outdated
Show resolved
Hide resolved
…f users/groups converted by Ranger userysnc as per given Regex
…f users/groups converted by Ranger userysnc as per given Regex
6d46c54
to
be624fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses the issue where Ranger plugins fail authorization due to differences in user/group name formatting between the source (e.g., LDAP/AD) and the Ranger Admin database. The changes introduce a shared utility (ugsync-util) and corresponding configuration constants to apply consistent name transformations on the plugin side. Key changes include:
- Updating various plugin assembly XML files to include the ugsync-util dependency.
- Enhancing the RangerDefaultRequestProcessor and RangerBasePlugin to apply case conversion and regex-based name transformations using a new Mapper instance.
- Adding new constants and a model (UgsyncNameTransformRules) to support the name transformation configuration.
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
distro/src/main/assembly/*.xml | Added for ugsync-util in several plugin and agent assembly definitions. |
agents-common/src/main/java/org/apache/ranger/plugin/util/ServicePolicies.java | Updated to copy service configuration including new name transformation settings. |
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerCommonConstants.java | Added new constants for name transformation configuration. |
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerDefaultRequestProcessor.java | Added logic to perform user and group name transformation based on config settings. |
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java | Introduced configuration loading for name transformation and Mapper instantiation. |
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPluginContext.java | Added getters and setters for transformation Mapper and case conversion strings. |
agents-common/src/main/java/org/apache/ranger/plugin/model/UgsyncNameTransformRules.java | New model to encapsulate name transformation rules. |
agents-common/pom.xml | Added dependency on ugsync-util with necessary exclusions. |
Comments suppressed due to low confidence (2)
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java:1288
- Consider using getDeclaredConstructor().newInstance() instead of newInstance() for instantiating the Mapper, to follow modern Java instantiation practices.
Mapper userNameRegExInst = regExClass.newInstance();
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java:1308
- Consider using getDeclaredConstructor().newInstance() instead of newInstance() for instantiating the Mapper, to follow modern Java instantiation practices.
Mapper groupNameRegExInst = regExClass.newInstance();
…f users/groups converted by Ranger userysnc as per given Regex
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
…f users/groups converted by Ranger userysnc as per given Regex
security-admin/src/main/java/org/apache/ranger/common/PropertiesUtil.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhavalshah9131 Please correct the JIRA description as it has Typo.
RANGER-5215 : Policy authorization fails for Ranger Plugins in case of users/groups converted by Ranger userysnc as per given Regex
…f users/groups converted by Ranger userysnc as per given Regex
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerDefaultRequestProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one more refactoring suggestion, but other than that it looks good
…f users/groups converted by Ranger userysnc as per given Regex
ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
Outdated
Show resolved
Hide resolved
…f users/groups converted by Ranger userysnc as per given Regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !!
…f users/groups converted by Ranger userysnc as per given Regex
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerDefaultRequestProcessor.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java
Outdated
Show resolved
Hide resolved
ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/transform/RegEx.java
Outdated
Show resolved
Hide resolved
…f users/groups converted by Ranger userysnc as per given Regex
ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/transform/RegEx.java
Outdated
Show resolved
Hide resolved
ugsync-util/src/test/java/org/apache/ranger/ugsynutil/transform/TestRegEx.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
Outdated
Show resolved
Hide resolved
@@ -194,7 +191,8 @@ public class ServiceREST { | |||
public static final String PURGE_RECORD_TYPE_LOGIN_LOGS = "login_records"; | |||
public static final String PURGE_RECORD_TYPE_TRX_LOGS = "trx_records"; | |||
public static final String PURGE_RECORD_TYPE_POLICY_EXPORT_LOGS = "policy_export_logs"; | |||
|
|||
private static final Logger LOG = LoggerFactory.getLogger(ServiceREST.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with rest of the sources, have initialization of Logger
instances at the beginning of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mneethiraj ,
LOG seems to be private. As per steps mentioned RANGER-5018 it gets shfited after public variables.
security-admin/src/main/java/org/apache/ranger/common/RangerServicePoliciesCache.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java
Outdated
Show resolved
Hide resolved
agents-common/src/main/java/org/apache/ranger/plugin/model/UgsyncNameTransformRules.java
Outdated
Show resolved
Hide resolved
…f users/groups converted by Ranger userysnc as per given Regex
ugsync-util/pom.xml
Outdated
@@ -47,6 +47,28 @@ | |||
<groupId>com.google.code.gson</groupId> | |||
<artifactId>gson</artifactId> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.apache.logging.log4j</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are libraries log4j-api
and log4j-over-slf4j
needed in ugsync-util module? If not, please remove these dependencies.
@@ -40,6 +41,10 @@ public class RangerPluginContext { | |||
private final RangerPluginConfig config; | |||
private final Map<String, Map<RangerPolicy.RangerPolicyResource, RangerResourceMatcher>> resourceMatchers = new HashMap<>(); | |||
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true); // fair lock | |||
private Mapper userNameTransformInst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving new members to RangerAuthContext
class.
@@ -1435,6 +1396,37 @@ public boolean isSyncSourceValidationEnabled() { | |||
return isSyncSourceValidationEnabled; | |||
} | |||
|
|||
public Map<String, String> getNameTransformationRules() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods getNameTransformationRules()
and getAllRegexPatternsConfig()
don't seem to be used. Please review and remove.
@@ -69,7 +62,7 @@ public String transform(String attrValue) { | |||
return result; | |||
} | |||
|
|||
protected void populateReplacementPatterns(String baseProperty, List<String> regexPatterns, String regexSeparator) { | |||
public void populateReplacementPatterns(String baseProperty, List<String> regexPatterns, String regexSeparator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referenced from this class, and its test class TestRegEx
. I suggest changing visibility to package-private i.e. remove public
. Btw, there is a typo in the test class package name: org.apache.ranger.ugsynutil.transform
=> org.apache.ranger.ugsyncutil.transform
(missing c
in ugsyncutil
).
LOG.warn("More than one character found in RegEx Separator, using default RegEx Separator"); | ||
} | ||
} | ||
LOG.info(String.format("Using %s as the RegEx Separator", ret)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace use of String.format
with parameterized log message.
String user = request.getUser(); | ||
String userNameCaseConversion = policyEngine.getPluginContext().getUserNameCaseConversion(); | ||
|
||
if (UgsyncCommonConstants.UGSYNC_LOWER_CASE_CONVERSION_VALUE.equalsIgnoreCase(userNameCaseConversion)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider avoiding string comparision on every authz call; instead have the configured parsed to a boolean in RangerAuthContext and access boolean from here.
.map(group -> { | ||
String originalGroup = group; | ||
|
||
if (UgsyncCommonConstants.UGSYNC_LOWER_CASE_CONVERSION_VALUE.equalsIgnoreCase(caseConversion)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider avoiding string comparision on every authz call, and for each group; instead have the configured parsed to a boolean in RangerAuthContext and access boolean from here.
Mapper groupNameTransformInst = policyEngine.getPluginContext().getGroupNameTransformInst(); | ||
String caseConversion = policyEngine.getPluginContext().getGroupNameCaseConversion(); | ||
|
||
if (groupNameTransformInst == null || request.getUserGroups() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When groupNameTransformInst
is null, return
at line 202 will result in case-conversion to be not applied. Please review and update.
What changes were proposed in this pull request?
Problem Statement:
Currently, when Ranger Usersync is configured with case conversion and special character replacement using regex, it transforms the original user/group names from the source (e.g., AD/LDAP) before storing them in the Ranger Admin database.
Example:
Original name in LDAP/AD: John-jacobs
Usersync configuration:
Issue:
If a Ranger plugin (e.g., Hive) uses the original name John-jacobs during authorization checks, it fails because Ranger Admin only recognizes the transformed name john_jacobs.
Error Example:
Permission denied: user [John-jacobs] does not have [SELECT] privilege on [vehicle/cars/*]
Solution:
To ensure consistency, the same transformation logic used by Usersync must also be applied on the plugin side before authorization. This transformation should be made available as a utility library packaged with the plugins.
Configurability:
This feature must be configurable at the plugin level via a property (e.g., ranger.plugin..supports.name.transformation), allowing users to enable or disable it based on their environment needs.
In ranger-admin-site.xml
ranger.plugins.ldap.username.caseconversion
ranger.plugins.ldap.groupname.caseconversion
ranger.plugins.mapping.username.handler
ranger.plugins.mapping.groupname.handler
ranger.plugins.mapping.regex.separator
ranger.plugins.mapping.username.regex
ranger.plugins.mapping.groupname.regex
How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests)
1.) Build successful with unit test.
2.) Manul testing