Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
> On Nov. 22, 2017, 2:35 p.m., Colm O hEigeartaigh wrote: > > Ship It! > > Abhay Kulkarni wrote: > All, > > Can we please hold on pushing this patch? I am waiting for input from > HDFS committers to ensure that this new HDFS authorization (Traverse > checking) call sequence is what is intendeded. Thanks! HDFS dev team responded as follows. "It looks like it is indeed a change of behaviour between 2.7 and 3.0. More specifically, HDFS-10997 introduced a change to FSDirectory#resolvePath, that when a file is accessed, this call will traversely ancestors, leading to an extra checkPermission() call. We don't plan to address this currently because this behavior sounds correct to me." Accordingly, I have updated the patch with some modifications, and posted another review (https://reviews.apache.org/r/64228). Please review and comment. Thanks! - Abhay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/#review191736 --- On Nov. 22, 2017, 12:39 p.m., Zsombor Gegesy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61062/ > --- > > (Updated Nov. 22, 2017, 12:39 p.m.) > > > Review request for ranger. > > > Bugs: RANGER-1707 > https://issues.apache.org/jira/browse/RANGER-1707 > > > Repository: ranger > > > Description > --- > > Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where > the traverse checks are called > before reading and writing files, so if a policy is just about reading > /tmp/somedir/somefile > it means, that traverse should be allowed to get to that file. Adding > more tests to highlight the issue > > > Diffs > - > > hdfs-agent/pom.xml 9f6206013 > > hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java > af4d9b5c2 > > hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61062/diff/3/ > > > Testing > --- > > Tested locally > https://travis-ci.org/gzsombor/ranger/builds/256331500 > > > Thanks, > > Zsombor Gegesy > >
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
> On Nov. 22, 2017, 2:35 p.m., Colm O hEigeartaigh wrote: > > Ship It! All, Can we please hold on pushing this patch? I am waiting for input from HDFS committers to ensure that this new HDFS authorization (Traverse checking) call sequence is what is intendeded. Thanks! - Abhay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/#review191736 --- On Nov. 22, 2017, 12:39 p.m., Zsombor Gegesy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61062/ > --- > > (Updated Nov. 22, 2017, 12:39 p.m.) > > > Review request for ranger. > > > Bugs: RANGER-1707 > https://issues.apache.org/jira/browse/RANGER-1707 > > > Repository: ranger > > > Description > --- > > Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where > the traverse checks are called > before reading and writing files, so if a policy is just about reading > /tmp/somedir/somefile > it means, that traverse should be allowed to get to that file. Adding > more tests to highlight the issue > > > Diffs > - > > hdfs-agent/pom.xml 9f6206013 > > hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java > af4d9b5c2 > > hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61062/diff/3/ > > > Testing > --- > > Tested locally > https://travis-ci.org/gzsombor/ranger/builds/256331500 > > > Thanks, > > Zsombor Gegesy > >
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/#review191736 --- Ship it! Ship It! - Colm O hEigeartaigh On Nov. 22, 2017, 12:39 p.m., Zsombor Gegesy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61062/ > --- > > (Updated Nov. 22, 2017, 12:39 p.m.) > > > Review request for ranger. > > > Bugs: RANGER-1707 > https://issues.apache.org/jira/browse/RANGER-1707 > > > Repository: ranger > > > Description > --- > > Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where > the traverse checks are called > before reading and writing files, so if a policy is just about reading > /tmp/somedir/somefile > it means, that traverse should be allowed to get to that file. Adding > more tests to highlight the issue > > > Diffs > - > > hdfs-agent/pom.xml 9f6206013 > > hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java > af4d9b5c2 > > hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61062/diff/3/ > > > Testing > --- > > Tested locally > https://travis-ci.org/gzsombor/ranger/builds/256331500 > > > Thanks, > > Zsombor Gegesy > >
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
> On Nov. 21, 2017, 4 p.m., Colm O hEigeartaigh wrote:
> > You could put some spaces into "for (int i=0;i > There's also an indentation issue on line 201 of RangerHdfsAuthorizerTest.
> > Other spacing issue here "ancestorIndex,plugin"
> >
> > > for (FsAction action : Arrays.asList(FsAction.EXECUTE, FsAction.READ,
> > > FsAction.WRITE)) {
> >
> > I think the FsAction.EXECUTE is not necessary here, as we are checking
> > EXECUTE already in "traverseOnlyCheck".
>
> Zsombor Gegesy wrote:
> The trick is, that there are different inodes used for the checks:
>
> final AuthzStatus status = isAccessAllowed(nodeToCheck, nodeAttribs,
> FsAction.EXECUTE, user, groups, plugin, auditHandler);
> if (status == AuthzStatus.NOT_DETERMINED) {
> return isAnyAccessAllowed(inode, inode, user, groups, plugin,
> auditHandler);
> }
>
> First, we use 'nodeToCheck', which can be a parent or ancestor node, and
> in the loop, we use 'inode' which refers to the actual file.
>
> Colm O hEigeartaigh wrote:
> OK understood thanks. The indentation issue is still there, now on line
> 224 of RangerHdfsAuthorizerTest (single tab character indent)
Thanks, I've fixed that too.
- Zsombor
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61062/#review191583
---
On Nov. 22, 2017, 12:39 p.m., Zsombor Gegesy wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61062/
> ---
>
> (Updated Nov. 22, 2017, 12:39 p.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-1707
> https://issues.apache.org/jira/browse/RANGER-1707
>
>
> Repository: ranger
>
>
> Description
> ---
>
> Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where
> the traverse checks are called
> before reading and writing files, so if a policy is just about reading
> /tmp/somedir/somefile
> it means, that traverse should be allowed to get to that file. Adding
> more tests to highlight the issue
>
>
> Diffs
> -
>
> hdfs-agent/pom.xml 9f6206013
>
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java
> af4d9b5c2
>
> hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61062/diff/3/
>
>
> Testing
> ---
>
> Tested locally
> https://travis-ci.org/gzsombor/ranger/builds/256331500
>
>
> Thanks,
>
> Zsombor Gegesy
>
>
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/ --- (Updated Nov. 22, 2017, 12:39 p.m.) Review request for ranger. Changes --- Space ... Bugs: RANGER-1707 https://issues.apache.org/jira/browse/RANGER-1707 Repository: ranger Description --- Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where the traverse checks are called before reading and writing files, so if a policy is just about reading /tmp/somedir/somefile it means, that traverse should be allowed to get to that file. Adding more tests to highlight the issue Diffs (updated) - hdfs-agent/pom.xml 9f6206013 hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java af4d9b5c2 hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/61062/diff/3/ Changes: https://reviews.apache.org/r/61062/diff/2-3/ Testing --- Tested locally https://travis-ci.org/gzsombor/ranger/builds/256331500 Thanks, Zsombor Gegesy
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
> On Nov. 21, 2017, 4 p.m., Colm O hEigeartaigh wrote:
> > You could put some spaces into "for (int i=0;i > There's also an indentation issue on line 201 of RangerHdfsAuthorizerTest.
> > Other spacing issue here "ancestorIndex,plugin"
> >
> > > for (FsAction action : Arrays.asList(FsAction.EXECUTE, FsAction.READ,
> > > FsAction.WRITE)) {
> >
> > I think the FsAction.EXECUTE is not necessary here, as we are checking
> > EXECUTE already in "traverseOnlyCheck".
>
> Zsombor Gegesy wrote:
> The trick is, that there are different inodes used for the checks:
>
> final AuthzStatus status = isAccessAllowed(nodeToCheck, nodeAttribs,
> FsAction.EXECUTE, user, groups, plugin, auditHandler);
> if (status == AuthzStatus.NOT_DETERMINED) {
> return isAnyAccessAllowed(inode, inode, user, groups, plugin,
> auditHandler);
> }
>
> First, we use 'nodeToCheck', which can be a parent or ancestor node, and
> in the loop, we use 'inode' which refers to the actual file.
OK understood thanks. The indentation issue is still there, now on line 224 of
RangerHdfsAuthorizerTest (single tab character indent)
- Colm
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61062/#review191583
---
On Nov. 21, 2017, 4:34 p.m., Zsombor Gegesy wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61062/
> ---
>
> (Updated Nov. 21, 2017, 4:34 p.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-1707
> https://issues.apache.org/jira/browse/RANGER-1707
>
>
> Repository: ranger
>
>
> Description
> ---
>
> Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where
> the traverse checks are called
> before reading and writing files, so if a policy is just about reading
> /tmp/somedir/somefile
> it means, that traverse should be allowed to get to that file. Adding
> more tests to highlight the issue
>
>
> Diffs
> -
>
> hdfs-agent/pom.xml 9f6206013
>
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java
> af4d9b5c2
>
> hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61062/diff/2/
>
>
> Testing
> ---
>
> Tested locally
> https://travis-ci.org/gzsombor/ranger/builds/256331500
>
>
> Thanks,
>
> Zsombor Gegesy
>
>
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
> On Nov. 21, 2017, 4 p.m., Colm O hEigeartaigh wrote:
> > You could put some spaces into "for (int i=0;i > There's also an indentation issue on line 201 of RangerHdfsAuthorizerTest.
> > Other spacing issue here "ancestorIndex,plugin"
> >
> > > for (FsAction action : Arrays.asList(FsAction.EXECUTE, FsAction.READ,
> > > FsAction.WRITE)) {
> >
> > I think the FsAction.EXECUTE is not necessary here, as we are checking
> > EXECUTE already in "traverseOnlyCheck".
The trick is, that there are different inodes used for the checks:
final AuthzStatus status = isAccessAllowed(nodeToCheck, nodeAttribs,
FsAction.EXECUTE, user, groups, plugin, auditHandler);
if (status == AuthzStatus.NOT_DETERMINED) {
return isAnyAccessAllowed(inode, inode, user, groups, plugin,
auditHandler);
}
First, we use 'nodeToCheck', which can be a parent or ancestor node, and in the
loop, we use 'inode' which refers to the actual file.
- Zsombor
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61062/#review191583
---
On Nov. 21, 2017, 4:34 p.m., Zsombor Gegesy wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61062/
> ---
>
> (Updated Nov. 21, 2017, 4:34 p.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-1707
> https://issues.apache.org/jira/browse/RANGER-1707
>
>
> Repository: ranger
>
>
> Description
> ---
>
> Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where
> the traverse checks are called
> before reading and writing files, so if a policy is just about reading
> /tmp/somedir/somefile
> it means, that traverse should be allowed to get to that file. Adding
> more tests to highlight the issue
>
>
> Diffs
> -
>
> hdfs-agent/pom.xml 9f6206013
>
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java
> af4d9b5c2
>
> hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61062/diff/2/
>
>
> Testing
> ---
>
> Tested locally
> https://travis-ci.org/gzsombor/ranger/builds/256331500
>
>
> Thanks,
>
> Zsombor Gegesy
>
>
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/ --- (Updated Nov. 21, 2017, 4:34 p.m.) Review request for ranger. Changes --- Javadoc, and whitespace changes Bugs: RANGER-1707 https://issues.apache.org/jira/browse/RANGER-1707 Repository: ranger Description --- Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where the traverse checks are called before reading and writing files, so if a policy is just about reading /tmp/somedir/somefile it means, that traverse should be allowed to get to that file. Adding more tests to highlight the issue Diffs (updated) - hdfs-agent/pom.xml 9f6206013 hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java af4d9b5c2 hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/61062/diff/2/ Changes: https://reviews.apache.org/r/61062/diff/1-2/ Testing --- Tested locally https://travis-ci.org/gzsombor/ranger/builds/256331500 Thanks, Zsombor Gegesy
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61062/#review191583
---
You could put some spaces into "for (int i=0;i for (FsAction action : Arrays.asList(FsAction.EXECUTE, FsAction.READ,
> FsAction.WRITE)) {
I think the FsAction.EXECUTE is not necessary here, as we are checking EXECUTE
already in "traverseOnlyCheck".
- Colm O hEigeartaigh
On July 22, 2017, 10:31 a.m., Zsombor Gegesy wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61062/
> ---
>
> (Updated July 22, 2017, 10:31 a.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-1707
> https://issues.apache.org/jira/browse/RANGER-1707
>
>
> Repository: ranger
>
>
> Description
> ---
>
> Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where
> the traverse checks are called
> before reading and writing files, so if a policy is just about reading
> /tmp/somedir/somefile
> it means, that traverse should be allowed to get to that file. Adding
> more tests to highlight the issue
>
>
> Diffs
> -
>
> hdfs-agent/pom.xml 9f62060
>
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java
> d28685a
>
> hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61062/diff/1/
>
>
> Testing
> ---
>
> Tested locally
> https://travis-ci.org/gzsombor/ranger/builds/256331500
>
>
> Thanks,
>
> Zsombor Gegesy
>
>
Re: Review Request 61062: RANGER-1707 : fix hdfs traverse check
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61062/#review182698 --- Looks OK to me, but I'd like to get some feedback from some of the other devs before applying it. - Colm O hEigeartaigh On July 22, 2017, 10:31 a.m., Zsombor Gegesy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61062/ > --- > > (Updated July 22, 2017, 10:31 a.m.) > > > Review request for ranger. > > > Bugs: RANGER-1707 > https://issues.apache.org/jira/browse/RANGER-1707 > > > Repository: ranger > > > Description > --- > > Fix hdfs traverse check, which problem was hidden before hdfs 2.8.0, where > the traverse checks are called > before reading and writing files, so if a policy is just about reading > /tmp/somedir/somefile > it means, that traverse should be allowed to get to that file. Adding > more tests to highlight the issue > > > Diffs > - > > hdfs-agent/pom.xml 9f62060 > > hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java > d28685a > > hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61062/diff/1/ > > > Testing > --- > > Tested locally > https://travis-ci.org/gzsombor/ranger/builds/256331500 > > > Thanks, > > Zsombor Gegesy > >
