Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RANGER-4670: RANGER-4977: hbase plugin: SELF_AND_ALL_DESCENDANTS Reso… #410

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

RANGER-4670: RANGER-4977: hbase plugin: SELF_AND_ALL_DESCENDANTS Reso… #410

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4e54841
RANGER-4670: RANGER-4977: hbase plugin: SELF_AND_ALL_DESCENDANTS Reso…
fateh288 Nov 4, 2024
0406897
Review comments: refactor for readability
fateh288 Nov 4, 2024
102cf9e
Review comments: make method static
fateh288 Nov 5, 2024
c9ee929
Review comments: code cleanup
fateh288 Nov 5, 2024
1d3fbca
Review comments: Handling null scope
fateh288 Nov 8, 2024
0df56ca
Merge branch 'master' into RANGER-4670
fateh288 Nov 12, 2024
487ef80
Review Comments: Use checkForDescendantDenial construct instead of SE…
fateh288 Nov 15, 2024
5f8e56c
cleanup
fateh288 Nov 15, 2024
00c6835
cleanup
fateh288 Nov 15, 2024
98a04ea
Added policy engine test cases for checkForDescendantDenial
fateh288 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...src/main/java/org/apache/ranger/authorization/hadoop/constants/RangerHadoopConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ public class RangerHadoopConstants {

public static final String HBASE_UPDATE_RANGER_POLICIES_ON_GRANT_REVOKE_PROP = "xasecure.hbase.update.xapolicies.on.grant.revoke";
public static final boolean HBASE_UPDATE_RANGER_POLICIES_ON_GRANT_REVOKE_DEFAULT_VALUE = true;

public static final String HBASE_COLUMN_AUTH_OPTIMIZATION = "ranger.plugin.hbase.column.auth.optimization";

public static final String KNOX_ACCESS_VERIFIER_CLASS_NAME_PROP = "knox.authorization.verifier.classname";
public static final String KNOX_ACCESS_VERIFIER_CLASS_NAME_DEFAULT_VALUE = "org.apache.ranger.pdp.knox.RangerAuthorizer";

Expand Down
2 changes: 1 addition & 1 deletion agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerAccessRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ default Map<String, ResourceElementMatchingScope> getResourceElementMatchingScop
return Collections.emptyMap();
}

enum ResourceMatchingScope { SELF, SELF_OR_DESCENDANTS }
enum ResourceMatchingScope { SELF, SELF_OR_DESCENDANTS, SELF_AND_ALL_DESCENDANTS }

enum ResourceElementMatchingScope { SELF, SELF_OR_CHILD, SELF_OR_PREFIX }

Expand Down
39 changes: 27 additions & 12 deletions .../src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,17 +231,8 @@ public void evaluate(RangerAccessRequest request, RangerAccessResult result) {
matchType = resourceMatcher != null ? resourceMatcher.getMatchType(request.getResource(), request.getResourceElementMatchingScopes(), request.getContext()) : RangerPolicyResourceMatcher.MatchType.NONE;
}

final boolean isMatched;

if (request.isAccessTypeAny()) {
isMatched = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
} else if (request.getResourceMatchingScope() == RangerAccessRequest.ResourceMatchingScope.SELF_OR_DESCENDANTS) {
isMatched = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
} else {
isMatched = matchType == RangerPolicyResourceMatcher.MatchType.SELF || matchType == RangerPolicyResourceMatcher.MatchType.SELF_AND_ALL_DESCENDANTS;
}

if (isMatched) {
final boolean isMatchedResource = isMatchForResourceMatchingScope(request.getResourceMatchingScope(), matchType, request.isAccessTypeAny());
if ( isMatchedResource ) {
//Evaluate Policy Level Custom Conditions, if any and allowed then go ahead for policyItem level evaluation
if (matchPolicyCustomConditions(request)) {
if (!result.getIsAuditedDetermined()) {
Expand Down Expand Up @@ -539,7 +530,8 @@ public void updateAccessResult(RangerAccessResult result, RangerPolicyResourceMa
LOG.debug("==> RangerDefaultPolicyEvaluator.updateAccessResult(" + result + ", " + matchType +", " + isAllowed + ", " + reason + ", " + getPolicyId() + ")");
}
if (!isAllowed) {
if (matchType != RangerPolicyResourceMatcher.MatchType.DESCENDANT) {
if (matchType != RangerPolicyResourceMatcher.MatchType.DESCENDANT ||
(result.getAccessRequest()!=null && result.getAccessRequest().getResourceMatchingScope() == RangerAccessRequest.ResourceMatchingScope.SELF_AND_ALL_DESCENDANTS)) {
result.setIsAllowed(false);
result.setPolicyPriority(getPolicyPriority());
result.setPolicyId(getPolicyId());
Expand Down Expand Up @@ -1512,4 +1504,27 @@ private List<RangerConditionEvaluator> createPolicyConditionEvaluators(RangerPol
return ret;
}

private static boolean isMatchForResourceMatchingScope(final RangerAccessRequest.ResourceMatchingScope scope, final RangerPolicyResourceMatcher.MatchType matchType, boolean isAnyMatch) {
boolean ret = false;
if (isAnyMatch){
ret = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
}
else if (scope!=null) {
switch (scope) {
case SELF_OR_DESCENDANTS: {
ret = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
mneethiraj marked this conversation as resolved.
Show resolved Hide resolved
break;
}
case SELF_AND_ALL_DESCENDANTS: {
ret = matchType != RangerPolicyResourceMatcher.MatchType.NONE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't matchType be SELF_AND_ALL_DESCENDENTS here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchType=DESCENDANT is the scenario we are also interested in.
if policy resource is (t1,cf1,c1) and accessed resource is (t1,cf1) then matchType is DESCENDANT which is what we want to match as it is a valid policy to evaluate for this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fateh288 - case blocks for SELF_OR_DESCENDANTS and SELF_AND_ALL_DESCENDANTS are the same, which doesn't look correct. Can you review?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 'case' blocks for both SELF_OR_DESCENDANTS and SELF_AND_ALL_DESCENDANTS would be the same.

ResourceMatchingScope=SELF_OR_DESCENDANTS will result in isMatched = true for all cases of matchType in {SELF, DESCENDANT, ANCESTOR, SELF_AND_ALL_DESCENDANTS} --- I am not sure here if matchType is Ancestor then why isMatched=true makes sense.
For ResourceMatchingScope=SELF_OR_DESCENDANTS, I tried changing
ret = matchType == RangerPolicyResourceMatcher.MatchType.SELF || matchType == RangerPolicyResourceMatcher.MatchType.SELF_AND_ALL_DESCENDANTS || matchType == RangerPolicyResourceMatcher.MatchType.DESCENDANT;

After this change, agents-common test cases pass successfully even after I change it to above (do you suggest changing it to this ?).

The same works for SELF_AND_ALL_DESCENDANTS too.

In the current implementation, the difference between SELF_OR_DESCENDANTS and SELF_AND_ALL_DESCENDANTS comes in implementation of updateAccessResult()

ranger/agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java

Line 533 in c9ee929

if (matchType != RangerPolicyResourceMatcher.MatchType.DESCENDANT ||

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj @kulkabhay
Any ideas on how to proceed?
I have resolved the merge conflicts.
Essentially with SELF_AND_ALL_DESCENDANTS we want same behavior as SELF_OR_DESCENDANTS but the special case for matchType!=DESCENDANTS needs to be bypassed

break;
}
default: {
ret = matchType == RangerPolicyResourceMatcher.MatchType.SELF || matchType == RangerPolicyResourceMatcher.MatchType.SELF_AND_ALL_DESCENDANTS;
break;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when scope is null, shouldn't the return be true if matchType is SELF or SELF_AND_ALL_DESCENDANTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope!=null always as per current implementation of RangerAccessRequestImpl .
private ResourceMatchingScope resourceMatchingScope = ResourceMatchingScope.SELF;
The default value is always SELF. But I see it can be somehow null if interface is implemented differently (I think we should prevent this)
Can there (or is there currently ) be a use case of null ResourceMatchingScope ?
However, I think a check for not null would be required for current refactoring logic as we cannot have null in switch case and we should return isMatched=False in this case unless there is an explicit use case for the same
(if ResourceMatchingScope is null then I think isMatched should be false as matchType seems irrelevant).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fateh288 - the regression in handling scope==null should be addressed in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I identified the issue is in test cases defined in test_policyengine_resource_with_req_expressions.json
The ResourceMatchingScope defined in these test cases is SELF_OR_CHILD which is not a valid value in the ResourceMatchingScope enum and results in null scope.

I tried changing ResourceMatchingScope to SELF here and all the test cases pass here. Do you suggest doing this ? Or should we add SELF_OR_CHILD as a valid ResourceMatchingScope ?

Yes, I can handle null as a scope or prevent regressions, but ideally it should be an invalid scenario if implemented correctly.

return ret;
}
}
6 changes: 6 additions & 0 deletions agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,14 +283,20 @@ public void testPolicyEngine_hbase_with_multiple_matching_policies() {

runTestsFromResourceFiles(hbaseTestResourceFiles);
}
@Test
public void testPolicyEngine_hbase_ResourceMatchingScope_equals_SELF_AND_ALL_DESCENDANTS() {
String[] hbaseTestResourceFiles = { "/policyengine/test_policyengine_hbase_selfandalldescendants.json" };

runTestsFromResourceFiles(hbaseTestResourceFiles);
}
@Test
public void testPolicyEngine_hbase_namespace() {
String[] hbaseTestResourceFiles = { "/policyengine/test_policyengine_hbase_namespace.json" };

runTestsFromResourceFiles(hbaseTestResourceFiles);
}


@Test
public void testPolicyEngine_hbaseForTag_filebased() {
String[] hbaseTestResourceFiles = { "/policyengine/test_policyengine_tag_hbase.json" };
Expand Down
56 changes: 56 additions & 0 deletions ...common/src/test/resources/policyengine/test_policyengine_hbase_selfandalldescendants.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
{
"serviceName":"hbasedev",

"serviceDef":{
"name":"hbase",
"id":2,
"resources":[
{"name":"table","level":1,"parent":"","mandatory":true,"lookupSupported":true,"matcher":"org.apache.ranger.plugin.resourcematcher.RangerDefaultResourceMatcher","matcherOptions":{"wildCard":true, "ignoreCase":true},"label":"HBase Table","description":"HBase Table"},
{"name":"column-family","level":2,"parent":"table","mandatory":true,"lookupSupported":true,"matcher":"org.apache.ranger.plugin.resourcematcher.RangerDefaultResourceMatcher","matcherOptions":{"wildCard":true, "ignoreCase":true},"label":"HBase Column-Family","description":"HBase Column-Family"},
{"name":"column","level":3,"parent":"column-family","mandatory":true,"lookupSupported":true,"matcher":"org.apache.ranger.plugin.resourcematcher.RangerDefaultResourceMatcher","matcherOptions":{"wildCard":true, "ignoreCase":true},"label":"HBase Column","description":"HBase Column"}
],
"accessTypes":[
{"name":"read","label":"Read"},
{"name":"write","label":"Write"},
{"name":"create","label":"Create"},
{"name":"admin","label":"Admin","impliedGrants":["read","write","create"]}
]
},

"policies":[
{"id":1,"name":"table=finance; column-family=restricted, column=restricted_column","isEnabled":true,"isAuditEnabled":true,
"resources":{"table":{"values":["finance"]},"column-family":{"values":["restricted_cf"]}, "column":{"values":["restricted_column"]}},
"denyPolicyItems":[
{"accesses":[{"type":"read","isAllowed":true},{"type":"read","isAllowed":true}],"users":["user1"],"groups":[],"delegateAdmin":false}
]
}
,
{"id":2,"name":"table=finance; column-family=restricted,column=*","isEnabled":true,"isAuditEnabled":true,
"resources":{"table":{"values":["finance"]},"column-family":{"values":["restricted_cf"]}, "column":{"values":["*"]}},
"policyItems":[
{"accesses":[{"type":"read","isAllowed":true},{"type":"read","isAllowed":true}],"users":["user1"],"groups":[],"delegateAdmin":false}
]
}
],

"tests":[
{"name":"TEST!!! DENY 'get' for restricted column family when ResourceMatchingScope=SELF_AND_ALL_DESCENDANTS",
"request":{
"resource":{"elements":{"table":"finance","column-family":"restricted_cf"}},
"resourceMatchingScope": "SELF_AND_ALL_DESCENDANTS",
"accessType":"read","user":"user1","requestData":"deny get as there is a restricted column"
},
"result":{"isAudited":true,"isAllowed":false,"policyId":1}
},
{"name":"TEST!!! Allow 'get' for restricted column family when ResourceMatchingScope=SELF",
"request":{
"resource":{"elements":{"table":"finance","column-family":"restricted_cf"}},
"resourceMatchingScope": "SELF",
"accessType":"read","user":"user1","requestData":"allow get as restricted column policy not considered"
},
"result":{"isAudited":true,"isAllowed":true,"policyId":2}
}

]
}

106 changes: 106 additions & 0 deletions agents-common/src/test/resources/resourcematcher/test_defaultpolicyresourcematcher.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,112 @@
"result": false
}
]
},
{
"name": "all * in policy, less number of resource elements in request",
"policyResources": {
"database": {"values": [ "*"] },
"table": {"values": ["*"]},
"column": {"values": ["*"]}
},
"tests": [
{
"name": "descendantMatch: NOT MATCH for database,table in policy",
"type": "descendantMatch",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": false
}
,
{
"name": "selfAndAllDescendants: MATCH for database,table in policy",
"type": "selfAndAllDescendants",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": true
}
]
},
{
"name": "multiple * in policy, less number of resource elements in request",
"policyResources": {
"database": {"values": [ "finance"] },
"table": {"values": ["*"]},
"column": {"values": ["*"]}
},
"tests": [
{
"name": "descendantMatch: NOT MATCH database,table in policy",
"type": "descendantMatch",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": false
}
,
{
"name": "selfAndAllDescendants: MATCH for database,table in policy",
"type": "selfAndAllDescendants",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": true
}
]
},
{
"name": "* in policy, less number of resource elements in request",
"policyResources": {
"database": {"values": [ "finance"] },
"table": {"values": ["tax"]},
"column": {"values": ["*"]}
},
"tests": [
{
"name": "descendantMatch: NOT MATCH for database,table in policy",
"type": "descendantMatch",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": false
}
,
{
"name": "selfAndAllDescendants: MATCH for database,table in policy",
"type": "selfAndAllDescendants",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": true
}
]
},
{
"name": "no * in policy, less number of resource elements in request",
"policyResources": {
"database": {"values": [ "finance"] },
"table": {"values": ["tax"]},
"column": {"values": ["ssn"]}
},
"tests": [
{
"name": "descendantMatch: MATCH for database,table in policy",
"type": "descendantMatch",
"resource": {
"elements": {"database": "finance", "table":"tax"}
},
"evalContext": {},
"result": true
}
]
}
]
}
40 changes: 22 additions & 18 deletions hbase-agent/src/main/java/org/apache/ranger/authorization/hbase/AuthorizationSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@
import org.apache.hadoop.thirdparty.com.google.common.collect.Sets;
import org.apache.hadoop.thirdparty.com.google.common.base.MoreObjects;
import org.apache.ranger.audit.model.AuthzAuditEvent;
import org.apache.ranger.plugin.policyengine.RangerAccessRequest;
import org.apache.ranger.plugin.policyengine.RangerAccessRequestImpl;
import org.apache.ranger.plugin.policyengine.RangerAccessResourceImpl;
import org.apache.ranger.plugin.policyengine.RangerAccessResult;
import org.apache.ranger.plugin.service.RangerBasePlugin;
import org.apache.ranger.plugin.policyengine.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -47,7 +43,7 @@ public class AuthorizationSession {
final HbaseUserUtils _userUtils = _factory.getUserUtils();
final HbaseAuthUtils _authUtils = _factory.getAuthUtils();
// immutable state
final RangerBasePlugin _authorizer;
final RangerHBasePlugin _authorizer;
// Mutable state: Use supplied state information
String _operation;
String _otherInformation;
Expand All @@ -68,7 +64,7 @@ public class AuthorizationSession {
RangerAccessRequest _request;
RangerAccessResult _result;

public AuthorizationSession(RangerBasePlugin authorizer) {
public AuthorizationSession(RangerHBasePlugin authorizer) {
_authorizer = authorizer;
}

Expand Down Expand Up @@ -172,37 +168,42 @@ boolean isNameSpaceOperation() {
StringUtils.equals(_operation, "getUserPermissionForNamespace");
}

AuthorizationSession buildRequest() {

verifyBuildable();
// session can be reused so reset its state
zapAuthorizationState();
private RangerAccessResource createHBaseResource() {
// TODO get this via a factory instead
RangerAccessResourceImpl resource = new RangerHBaseResource();
// policy engine should deal sensibly with null/empty values, if any
if (isNameSpaceOperation() && StringUtils.isNotBlank(_otherInformation)) {
resource.setValue(RangerHBaseResource.KEY_TABLE, _otherInformation + RangerHBaseResource.NAMESPACE_SEPARATOR);
resource.setValue(RangerHBaseResource.KEY_TABLE, _otherInformation + RangerHBaseResource.NAMESPACE_SEPARATOR);
} else {
resource.setValue(RangerHBaseResource.KEY_TABLE, _table);
}
resource.setValue(RangerHBaseResource.KEY_COLUMN_FAMILY, _columnFamily);
resource.setValue(RangerHBaseResource.KEY_COLUMN, _column);

return resource;
}
private RangerAccessRequest createRangerRequest() {
RangerAccessResource resource = createHBaseResource();
String user = _userUtils.getUserAsString(_user);
RangerAccessRequestImpl request = new RangerAccessRequestImpl(resource, _access, user, _groups, null);
request.setAction(_operation);
request.setRequestData(_otherInformation);
request.setClientIPAddress(_remoteAddress);
request.setResourceMatchingScope(_resourceMatchingScope);
request.setAccessTime(new Date());

_request = request;
return request;
}

AuthorizationSession buildRequest() {
verifyBuildable();
// session can be reused so reset its state
zapAuthorizationState();
_request = createRangerRequest();
if (LOG.isDebugEnabled()) {
LOG.debug("Built request: " + request.toString());
LOG.debug("Built request: " + _request.toString());
}
return this;
}

AuthorizationSession authorize() {
if (LOG.isDebugEnabled()) {
LOG.debug("==> AuthorizationSession.authorize: " + getRequestMessage());
Expand Down Expand Up @@ -377,4 +378,7 @@ AuthorizationSession resourceMatchingScope(RangerAccessRequest.ResourceMatchingS
_resourceMatchingScope = scope;
return this;
}
public boolean getPropertyIsColumnAuthOptimizationEnabled() {
return _authorizer.getPropertyIsColumnAuthOptimizationEnabled();
}
}
Loading