RANGER-4670: RANGER-4977: hbase plugin: SELF_AND_ALL_DESCENDANTS Reso… #410
Conversation
…urceMatchingScope, support shortcircuit column auth
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
when scope is null, shouldn't the return be true if matchType is SELF or SELF_AND_ALL_DESCENDANTS?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@fateh288 - the regression in handling scope==null should be addressed in this PR.
There was a problem hiding this comment.
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.
| break; | ||
| } | ||
| case SELF_AND_ALL_DESCENDANTS: { | ||
| ret = matchType != RangerPolicyResourceMatcher.MatchType.NONE; |
There was a problem hiding this comment.
shouldn't matchType be SELF_AND_ALL_DESCENDENTS here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@fateh288 - case blocks for SELF_OR_DESCENDANTS and SELF_AND_ALL_DESCENDANTS are the same, which doesn't look correct. Can you review?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
@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
...mon/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java
Outdated
Show resolved
Hide resolved
fateh288
force-pushed
the
RANGER-4670
branch
from
29c98c5
to
9862f14
Compare
fateh288
force-pushed
the
RANGER-4670
branch
from
9862f14
to
98a04ea
Compare
…urceMatchingScope, support shortcircuit column auth
What changes were proposed in this pull request?
RANGER-4977: Introduced a new SELF_AND_ALL_DESCENDANTS ResourceMatchingScope which is now used for column family authorization which fixes bug in scan operation wherein a denied column was also being returned in the scan operation.
RANGER-4670: A property/flag can be set via hbase service configs wherein during column level authorization, it can be used to determine if an authorization at column family can be used to determine if column authorization can be shortcircuited as an optimization (there will be audit behavior changes in this case). This is useful for improving performance especially for multiget and multiput workloads wherein thousands of columns can come in the authorization request and just checking column family level access can improve performance drastically
How was this patch tested?
New unit test cases have been added for the policy engine for the new ResourceMatchingScope of SELF_AND_ALL_DESCENDANTS.
New test cases have also been added for hbase. The test cases cover the scan operation bug and also covers new unit test cases for testing correctness of column authorization shortcircuiting. These test cases do not work on master branch (because they are ignored using junit ignore annotation) currently https://issues.apache.org/jira/browse/RANGER-4686 but I have verified that they work on my private fork.