You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Abhay Kulkarni <ak...@hortonworks.com> on 2022/12/02 18:39:26 UTC
Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/
-----------------------------------------------------------
Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
Bugs: RANGER-3995
https://issues.apache.org/jira/browse/RANGER-3995
Repository: ranger
Description
-------
Steps to reproduce :-
1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
/service/public/v2/api/policy/apply
2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
Diffs
-----
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
Diff: https://reviews.apache.org/r/74229/diff/1/
Testing
-------
Tested per the steps listed above. No error was reported and the policy was applied correctly
Thanks,
Abhay Kulkarni
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 3, 2022, 7:34 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Lines 228 (patched)
> > <https://reviews.apache.org/r/74229/diff/2/?file=2272258#file2272258line230>
> >
> > if appliedPolicy has isDenyAllElse=true and existingPolicy has isDenyAllElse=false, it should be treated as an error - as it can result in access to be denied for users who might have access currently. Please reveiew.
>
> Abhay Kulkarni wrote:
> I don't quite see why. Can you please elaborate? Thanks!
Consider following:
Existing policies:
- #1: resource: { database: fin, table: * } groups: [ fin-admin ], accessTypes: [ select ], isDenyAllElse: false
- #2: resource: { database: fin, table: t1 } groups: [ fin-user ], accessTypes: [ select ], isDenyAllElse: false
Applied policy:
- resource: { database: fin, table: t1 } users: [ john ], accessTypes: [ select ], isDenyAllElse: true
If above applyPolicy() changes isDenyAllElse to true on #2, it might result in fin-admin group to loose access to fin.t1 table.
My suggestion is to ignore isDenyAllElse coming from applyPolicy() - even if a new policy is created as result of applyPolicy().
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224948
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
> On Dec. 3, 2022, 7:34 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Lines 228 (patched)
> > <https://reviews.apache.org/r/74229/diff/2/?file=2272258#file2272258line230>
> >
> > if appliedPolicy has isDenyAllElse=true and existingPolicy has isDenyAllElse=false, it should be treated as an error - as it can result in access to be denied for users who might have access currently. Please reveiew.
I don't quite see why. Can you please elaborate? Thanks!
- Abhay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224948
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224948
-----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
Lines 228 (patched)
<https://reviews.apache.org/r/74229/#comment313787>
if appliedPolicy has isDenyAllElse=true and existingPolicy has isDenyAllElse=false, it should be treated as an error - as it can result in access to be denied for users who might have access currently. Please reveiew.
- Madhan Neethiraj
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224961
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On Dec. 6, 2022, 3:36 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2022, 3:36 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 9bccf1089
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/3/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/
-----------------------------------------------------------
(Updated Dec. 6, 2022, 3:36 a.m.)
Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
Changes
-------
Addressed review comments.
Bugs: RANGER-3995
https://issues.apache.org/jira/browse/RANGER-3995
Repository: ranger
Description
-------
Steps to reproduce :-
1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
/service/public/v2/api/policy/apply
2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
Diffs (updated)
-----
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 9bccf1089
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
Diff: https://reviews.apache.org/r/74229/diff/3/
Changes: https://reviews.apache.org/r/74229/diff/2-3/
Testing
-------
Tested per the steps listed above. No error was reported and the policy was applied correctly
Thanks,
Abhay Kulkarni
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/
-----------------------------------------------------------
(Updated Dec. 3, 2022, 12:28 a.m.)
Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
Changes
-------
Fixed a review comment
Bugs: RANGER-3995
https://issues.apache.org/jira/browse/RANGER-3995
Repository: ranger
Description
-------
Steps to reproduce :-
1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
/service/public/v2/api/policy/apply
2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
Diffs (updated)
-----
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
Diff: https://reviews.apache.org/r/74229/diff/2/
Changes: https://reviews.apache.org/r/74229/diff/1-2/
Testing
-------
Tested per the steps listed above. No error was reported and the policy was applied correctly
Thanks,
Abhay Kulkarni
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Line 608 (original), 628 (patched)
> > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637>
> >
> > addition to deny-exception can/must be skipped in following cases:
> > 1. policy has no deny policy items
> > 2. policy has denyAllElse=true
> >
> > Case #1 is covered by #629.
> >
> > To cover #1, how about updating processApplyPolicyForItemType() as below:
> >
> > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
> >
> > if (policyItemType == ALLOW) {
> > appliedPolicyItems = appliedPolicy.getPolicyItems();
> > } else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
> > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
> > } else {
> > if (policyItemType == DENY) {
> > appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
> > } else if (policyItemType == ALLOW_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getAllowExceptions();
> > } else if (policyItemType == DENY_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getDenyExceptions();
> > } else {
> > LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
> > }
> > }
I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy.
- Abhay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
On Dec. 2, 2022, 6:39 p.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2022, 6:39 p.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/1/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Line 608 (original), 628 (patched)
> > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637>
> >
> > addition to deny-exception can/must be skipped in following cases:
> > 1. policy has no deny policy items
> > 2. policy has denyAllElse=true
> >
> > Case #1 is covered by #629.
> >
> > To cover #1, how about updating processApplyPolicyForItemType() as below:
> >
> > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
> >
> > if (policyItemType == ALLOW) {
> > appliedPolicyItems = appliedPolicy.getPolicyItems();
> > } else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
> > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
> > } else {
> > if (policyItemType == DENY) {
> > appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
> > } else if (policyItemType == ALLOW_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getAllowExceptions();
> > } else if (policyItemType == DENY_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getDenyExceptions();
> > } else {
> > LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
> > }
> > }
>
> Abhay Kulkarni wrote:
> I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy.
>
> Madhan Neethiraj wrote:
> 1. When a policy has isDenyAllElse=true, deny and deny-exceptions are not relevant, hence it is safe to ignore them. May be such requests should be failed?
> 2. is there a need for applyPolicy to support isDenyAllElse=true and deny/exceptions?
>
> Abhay Kulkarni wrote:
> 1. True, but I think it is preferably to fail the request instead of quietly ignoring deny/denyException policyItems quietly.
> 2. No.
>
> Madhan Neethiraj wrote:
> yes, applyPolicy() should fail such requests. Does the current patch fail these?
Yes, it would fail in the updatePolicy() call that is made to save the policy after applyPolicy() logic is executed.
- Abhay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Line 608 (original), 628 (patched)
> > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637>
> >
> > addition to deny-exception can/must be skipped in following cases:
> > 1. policy has no deny policy items
> > 2. policy has denyAllElse=true
> >
> > Case #1 is covered by #629.
> >
> > To cover #1, how about updating processApplyPolicyForItemType() as below:
> >
> > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
> >
> > if (policyItemType == ALLOW) {
> > appliedPolicyItems = appliedPolicy.getPolicyItems();
> > } else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
> > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
> > } else {
> > if (policyItemType == DENY) {
> > appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
> > } else if (policyItemType == ALLOW_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getAllowExceptions();
> > } else if (policyItemType == DENY_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getDenyExceptions();
> > } else {
> > LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
> > }
> > }
>
> Abhay Kulkarni wrote:
> I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy.
1. When a policy has isDenyAllElse=true, deny and deny-exceptions are not relevant, hence it is safe to ignore them. May be such requests should be failed?
2. is there a need for applyPolicy to support isDenyAllElse=true and deny/exceptions?
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Abhay Kulkarni <ak...@hortonworks.com>.
> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Line 608 (original), 628 (patched)
> > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637>
> >
> > addition to deny-exception can/must be skipped in following cases:
> > 1. policy has no deny policy items
> > 2. policy has denyAllElse=true
> >
> > Case #1 is covered by #629.
> >
> > To cover #1, how about updating processApplyPolicyForItemType() as below:
> >
> > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
> >
> > if (policyItemType == ALLOW) {
> > appliedPolicyItems = appliedPolicy.getPolicyItems();
> > } else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
> > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
> > } else {
> > if (policyItemType == DENY) {
> > appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
> > } else if (policyItemType == ALLOW_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getAllowExceptions();
> > } else if (policyItemType == DENY_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getDenyExceptions();
> > } else {
> > LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
> > }
> > }
>
> Abhay Kulkarni wrote:
> I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy.
>
> Madhan Neethiraj wrote:
> 1. When a policy has isDenyAllElse=true, deny and deny-exceptions are not relevant, hence it is safe to ignore them. May be such requests should be failed?
> 2. is there a need for applyPolicy to support isDenyAllElse=true and deny/exceptions?
1. True, but I think it is preferably to fail the request instead of quietly ignoring deny/denyException policyItems quietly.
2. No.
- Abhay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
> > Line 608 (original), 628 (patched)
> > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637>
> >
> > addition to deny-exception can/must be skipped in following cases:
> > 1. policy has no deny policy items
> > 2. policy has denyAllElse=true
> >
> > Case #1 is covered by #629.
> >
> > To cover #1, how about updating processApplyPolicyForItemType() as below:
> >
> > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
> >
> > if (policyItemType == ALLOW) {
> > appliedPolicyItems = appliedPolicy.getPolicyItems();
> > } else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
> > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
> > } else {
> > if (policyItemType == DENY) {
> > appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
> > } else if (policyItemType == ALLOW_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getAllowExceptions();
> > } else if (policyItemType == DENY_EXCEPTIONS) {
> > appliedPolicyItems = appliedPolicy.getDenyExceptions();
> > } else {
> > LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
> > }
> > }
>
> Abhay Kulkarni wrote:
> I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy.
>
> Madhan Neethiraj wrote:
> 1. When a policy has isDenyAllElse=true, deny and deny-exceptions are not relevant, hence it is safe to ignore them. May be such requests should be failed?
> 2. is there a need for applyPolicy to support isDenyAllElse=true and deny/exceptions?
>
> Abhay Kulkarni wrote:
> 1. True, but I think it is preferably to fail the request instead of quietly ignoring deny/denyException policyItems quietly.
> 2. No.
yes, applyPolicy() should fail such requests. Does the current patch fail these?
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
On Dec. 3, 2022, 12:28 a.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2022, 12:28 a.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/2/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>
Re: Review Request 74229: RANGER-3995: Policy update request fails if isDenyAllElse flag is set true in request json when using "/policy/apply" API
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74229/#review224943
-----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
Lines 274 (patched)
<https://reviews.apache.org/r/74229/#comment313782>
Shouldn't appliedPolicyItem be added to itemsToAdd only if none of the entries in existingPolicyItems match appliedPolicyItem? i.e. replace #273 - #277 with the following:
if (!existingPolicyItems.contains(appliedPolicyItem)) {
itemsToAdd.add(appliedPolicyItem);
}
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java
Line 608 (original), 628 (patched)
<https://reviews.apache.org/r/74229/#comment313783>
addition to deny-exception can/must be skipped in following cases:
1. policy has no deny policy items
2. policy has denyAllElse=true
Case #1 is covered by #629.
To cover #1, how about updating processApplyPolicyForItemType() as below:
List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null;
if (policyItemType == ALLOW) {
appliedPolicyItems = appliedPolicy.getPolicyItems();
} else if (existingPolicy.getIsDenyAllElse() || appliedPolicy.getIsDenyAllElse()) {
LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, ignoring invalid policyItemType=" + policyItemType);
} else {
if (policyItemType == DENY) {
appliedPolicyItems = appliedPolicy.getDenyPolicyItems();
} else if (policyItemType == ALLOW_EXCEPTIONS) {
appliedPolicyItems = appliedPolicy.getAllowExceptions();
} else if (policyItemType == DENY_EXCEPTIONS) {
appliedPolicyItems = appliedPolicy.getDenyExceptions();
} else {
LOG.warn("processApplyPolicyForItemType(): ignoring invalid policyItemType=" + policyItemType);
}
}
- Madhan Neethiraj
On Dec. 2, 2022, 6:39 p.m., Abhay Kulkarni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74229/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2022, 6:39 p.m.)
>
>
> Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3995
> https://issues.apache.org/jira/browse/RANGER-3995
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Steps to reproduce :-
>
> 1. Make a POST request to the below mentioned API endpoint, using a policy json where isDenyAllElse flag is set true
>
> /service/public/v2/api/policy/apply
> 2. Fetch the policy using the newly created policy id, and try to make a POST request to "/policy/apply" using the policy json obtained from the GET request. The request results in an error
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java b56fd3966
>
>
> Diff: https://reviews.apache.org/r/74229/diff/1/
>
>
> Testing
> -------
>
> Tested per the steps listed above. No error was reported and the policy was applied correctly
>
>
> Thanks,
>
> Abhay Kulkarni
>
>