Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-8312

Trash does not descent into child directories to check for permissions

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.0, 2.6.0, 2.7.2
    • Fix Version/s: 2.9.0, 2.7.4, 3.0.0-alpha1, 2.8.1
    • Component/s: fs, security
    • Labels:
      None
    • Target Version/s:
    • Release Note:
      Hide
      HDFS-8312. Added permission check for moving file to Trash. (Weiwei Yang via Eric Yang)
      Show
      HDFS-8312 . Added permission check for moving file to Trash. (Weiwei Yang via Eric Yang)

      Description

      HDFS trash does not descent into child directory to check if user has permission to delete files. For example:

      Run the following command to initialize directory structure as super user:

      hadoop fs -mkdir /BSS/level1
      hadoop fs -mkdir /BSS/level1/level2
      hadoop fs -mkdir /BSS/level1/level2/level3
      hadoop fs -put /tmp/appConfig.json /BSS/level1/level2/level3/testfile.txt
      hadoop fs -chown user1:users /BSS/level1/level2/level3/testfile.txt
      hadoop fs -chown -R user1:users /BSS/level1
      hadoop fs -chown -R 750 /BSS/level1
      hadoop fs -chmod -R 640 /BSS/level1/level2/level3/testfile.txt
      hadoop fs -chmod 775 /BSS
      

      Change to a normal user called user2.

      When trash is enabled:

      sudo su user2 -
      hadoop fs -rm -r /BSS/level1
      15/05/01 16:51:20 INFO fs.TrashPolicyDefault: Namenode trash configuration: Deletion interval = 3600 minutes, Emptier interval = 0 minutes.
      Moved: 'hdfs://bdvs323.svl.ibm.com:9000/BSS/level1' to trash at: hdfs://bdvs323.svl.ibm.com:9000/user/user2/.Trash/Current
      

      When trash is disabled:

      /opt/ibm/biginsights/IHC/bin/hadoop fs -Dfs.trash.interval=0 -rm -r /BSS/level1
      15/05/01 16:58:31 INFO fs.TrashPolicyDefault: Namenode trash configuration: Deletion interval = 0 minutes, Emptier interval = 0 minutes.
      rm: Permission denied: user=user2, access=ALL, inode="/BSS/level1":user1:users:drwxr-x---
      

      There is inconsistency between trash behavior and delete behavior. When trash is enabled, files owned by user1 is deleted by user2. It looks like trash does not recursively validate if the child directory files can be removed.

      1. HDFS-8312-001.patch
        11 kB
        Weiwei Yang
      2. HDFS-8312-002.patch
        11 kB
        Weiwei Yang
      3. HDFS-8312-003.patch
        11 kB
        Weiwei Yang
      4. HDFS-8312-004.patch
        11 kB
        Weiwei Yang
      5. HDFS-8312-005.patch
        11 kB
        Weiwei Yang
      6. HDFS-8312-branch-2.7.patch
        12 kB
        Brahma Reddy Battula
      7. HDFS-8312-branch-2.8.01.patch
        12 kB
        Weiwei Yang
      8. HDFS-8312-branch-2.8.1.001.patch
        12 kB
        Weiwei Yang
      9. HDFS-8312-testcase.patch
        4 kB
        Weiwei Yang

        Issue Links

          Activity

          Hide
          luisfernandoantonioli Luis Fernando Antonioli added a comment -

          I have been able to reproduce the bug in Hadoop 2.6.0, but not by following the same steps you proposed. Following your steps, I got permission denied in both cases and could not delete the files. In my test, I used the super user account to create a shared folder (every user can upload files to this directory) in the root directory of the HDFS and then used two different non root accounts (user1 and user2) to upload files to this folder (one user does not have permission to edit the files of the other). Finally, I could reproduce the inconsistency. When the HDFS thash was disabled, I got permission denied when trying to delete the files with one of the non root accounts and when the trash was enabled I was able to move all the files to the trash folder. Although I cannot delete the files directly from the trash folder, they will be deleted when the deletion interval set in the Hadoop configuration is reached. I could not reproduce this issue in Hadoop 2.7.1, I got permission denied in both cases. I think this bug was fixed in newer versions of Hadoop.

          Show
          luisfernandoantonioli Luis Fernando Antonioli added a comment - I have been able to reproduce the bug in Hadoop 2.6.0, but not by following the same steps you proposed. Following your steps, I got permission denied in both cases and could not delete the files. In my test, I used the super user account to create a shared folder (every user can upload files to this directory) in the root directory of the HDFS and then used two different non root accounts (user1 and user2) to upload files to this folder (one user does not have permission to edit the files of the other). Finally, I could reproduce the inconsistency. When the HDFS thash was disabled, I got permission denied when trying to delete the files with one of the non root accounts and when the trash was enabled I was able to move all the files to the trash folder. Although I cannot delete the files directly from the trash folder, they will be deleted when the deletion interval set in the Hadoop configuration is reached. I could not reproduce this issue in Hadoop 2.7.1, I got permission denied in both cases. I think this bug was fixed in newer versions of Hadoop.
          Hide
          cheersyang Weiwei Yang added a comment - - edited

          Hello Eric Yang and Luis Fernando Antonioli

          I was able to reproduce this issue with following steps

          I created user1 and user2, they both belongs to users group

          uid=1007(user1) gid=1007(user1) groups=1007(user1),100(users)
          uid=1009(user2) gid=1010(user2) groups=1010(user2),100(users)
          

          As super user,

          su hdfs
          hadoop fs -mkdir /BSS
          hadoop fs -chmod 777 /BSS
          

          As user1

          su user1
          hadoop fs -mkdir /BSS/user1/
          hadoop fs -put /tmp/test /BSS/user1/
          hadoop chmod 600 /BSS/user1/test
          
          hdfs dfs -ls -R /BSS
          drwxr-xr-x   - user1 users          0 2016-08-11 18:06 /BSS/user1
          -rw-------   3 user1 users       4308 2016-08-11 18:06 /BSS/user1/test
          

          As user2, try to delete /BSS/user1 directory with -skipTrash, it gives permission denied error (expected)

          su user2
          hadoop dfs -rmr -skipTrash /BSS/user1/
          rmr: Permission denied: user=user2, access=ALL, inode="/BSS/user1":user1:users:drwxr-xr-x
          

          try to delete the directory again without disabling trash,

          su user2
          hadoop dfs -rmr /BSS/user1/
          16/08/04 01:47:53 INFO fs.TrashPolicyDefault: Namenode trash configuration: Deletion interval = 360 minutes, Emptier interval = 0 minutes.
          Moved: 'hdfs://bdavm1542.svl.ibm.com:8020/BSS/user1' to trash at: hdfs://bdavm1542.svl.ibm.com:8020/user/user2/.Trash/Current
          

          directory removed (unexpected).

          This opens security hole that user is able to delete other user files, this should be fixed. Agree ?

          Internally, trash calls rename and rm calls delete, that's why the behavior was not consistent, user2 has the permission to move the directory but not to delete

          su user2
          hadoop dfs -mv /BSS/user1 /user/user2/.Trash
          hadoop dfs -ls /user/user2/.Trash
          Found 2 items
          drwx------   - user2 hdfs           0 2016-08-04 01:39 /user/user2/.Trash/Current
          drwxr-xr-x   - user1 users          0 2016-08-04 01:59 /user/user2/.Trash/user1
          

          To fix this, when trash enabled, even it runs rename, we need to check delete permission because that is the intention (and the result) of the command. Assigned to myself to work on.

          Show
          cheersyang Weiwei Yang added a comment - - edited Hello Eric Yang and Luis Fernando Antonioli I was able to reproduce this issue with following steps I created user1 and user2, they both belongs to users group uid=1007(user1) gid=1007(user1) groups=1007(user1),100(users) uid=1009(user2) gid=1010(user2) groups=1010(user2),100(users) As super user, su hdfs hadoop fs -mkdir /BSS hadoop fs -chmod 777 /BSS As user1 su user1 hadoop fs -mkdir /BSS/user1/ hadoop fs -put /tmp/test /BSS/user1/ hadoop chmod 600 /BSS/user1/test hdfs dfs -ls -R /BSS drwxr-xr-x - user1 users 0 2016-08-11 18:06 /BSS/user1 -rw------- 3 user1 users 4308 2016-08-11 18:06 /BSS/user1/test As user2, try to delete /BSS/user1 directory with -skipTrash, it gives permission denied error (expected) su user2 hadoop dfs -rmr -skipTrash /BSS/user1/ rmr: Permission denied: user=user2, access=ALL, inode= "/BSS/user1" :user1:users:drwxr-xr-x try to delete the directory again without disabling trash, su user2 hadoop dfs -rmr /BSS/user1/ 16/08/04 01:47:53 INFO fs.TrashPolicyDefault: Namenode trash configuration: Deletion interval = 360 minutes, Emptier interval = 0 minutes. Moved: 'hdfs: //bdavm1542.svl.ibm.com:8020/BSS/user1' to trash at: hdfs://bdavm1542.svl.ibm.com:8020/user/user2/.Trash/Current directory removed (unexpected). This opens security hole that user is able to delete other user files, this should be fixed. Agree ? Internally, trash calls rename and rm calls delete , that's why the behavior was not consistent, user2 has the permission to move the directory but not to delete su user2 hadoop dfs -mv /BSS/user1 /user/user2/.Trash hadoop dfs -ls /user/user2/.Trash Found 2 items drwx------ - user2 hdfs 0 2016-08-04 01:39 /user/user2/.Trash/Current drwxr-xr-x - user1 users 0 2016-08-04 01:59 /user/user2/.Trash/user1 To fix this, when trash enabled, even it runs rename, we need to check delete permission because that is the intention (and the result) of the command. Assigned to myself to work on.
          Hide
          cheersyang Weiwei Yang added a comment -

          Attached a test case demonstrated this issue, see HDFS-8312-testcase.patch. Also thinking on how to fix it. There are generally two options

          1) Trash now calls FileSystem.rename to move file/dir to trash dir, so it only checks rename permission. A fix is to add a new method to check if delete is permitted, expose that from FileSystem API so we can check if user has permission to delete before rename in trash code.

          2) Improve Emptier code logic to let emptier run per user, so even user removes somebody else stuff to trash, the emptier will still not be able to remove it because it's not permitted by this user. This is better than delete ...

          I personal prefer option 1 because 2 looks like a partial fix, we should avoid user moving things to trash if it is not allowed to at first place.

          Any suggestions ? Appreciate!

          Show
          cheersyang Weiwei Yang added a comment - Attached a test case demonstrated this issue, see HDFS-8312-testcase.patch . Also thinking on how to fix it. There are generally two options 1) Trash now calls FileSystem.rename to move file/dir to trash dir, so it only checks rename permission. A fix is to add a new method to check if delete is permitted, expose that from FileSystem API so we can check if user has permission to delete before rename in trash code. 2) Improve Emptier code logic to let emptier run per user, so even user removes somebody else stuff to trash, the emptier will still not be able to remove it because it's not permitted by this user. This is better than delete ... I personal prefer option 1 because 2 looks like a partial fix, we should avoid user moving things to trash if it is not allowed to at first place. Any suggestions ? Appreciate!
          Hide
          eyang Eric Yang added a comment -

          Option 1 sounds more correct implementation. Moving or renaming files must check permission of the full path before the operation is committed.

          Show
          eyang Eric Yang added a comment - Option 1 sounds more correct implementation. Moving or renaming files must check permission of the full path before the operation is committed.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 17s Maven dependency ordering for branch
          +1 mvninstall 7m 14s trunk passed
          +1 compile 7m 12s trunk passed
          +1 checkstyle 1m 28s trunk passed
          +1 mvnsite 2m 35s trunk passed
          +1 mvneclipse 0m 47s trunk passed
          +1 findbugs 4m 40s trunk passed
          +1 javadoc 2m 6s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 2m 2s the patch passed
          +1 compile 9m 10s the patch passed
          +1 cc 9m 10s the patch passed
          +1 javac 9m 10s the patch passed
          +1 checkstyle 1m 31s the patch passed
          +1 mvnsite 2m 53s the patch passed
          +1 mvneclipse 0m 42s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 53s the patch passed
          +1 javadoc 2m 4s the patch passed
          -1 unit 7m 45s hadoop-common in the patch failed.
          +1 unit 0m 58s hadoop-hdfs-client in the patch passed.
          -1 unit 76m 55s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          138m 17s



          Reason Tests
          Failed junit tests hadoop.fs.viewfs.TestViewFsTrash
            hadoop.fs.TestTrash
            hadoop.hdfs.TestDFSPermission
            hadoop.hdfs.TestDFSShell
            hadoop.hdfs.TestHDFSTrash
            hadoop.hdfs.server.namenode.ha.TestHAAppend
            hadoop.hdfs.server.namenode.TestDecommissioningStatus
            hadoop.hdfs.TestEncryptionZones
            hadoop.hdfs.TestEncryptionZonesWithKMS



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823813/HDFS-8312-001.patch
          JIRA Issue HDFS-8312
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux c12a826255c2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 02abd13
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16433/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16433/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16433/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16433/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 17s Maven dependency ordering for branch +1 mvninstall 7m 14s trunk passed +1 compile 7m 12s trunk passed +1 checkstyle 1m 28s trunk passed +1 mvnsite 2m 35s trunk passed +1 mvneclipse 0m 47s trunk passed +1 findbugs 4m 40s trunk passed +1 javadoc 2m 6s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 2m 2s the patch passed +1 compile 9m 10s the patch passed +1 cc 9m 10s the patch passed +1 javac 9m 10s the patch passed +1 checkstyle 1m 31s the patch passed +1 mvnsite 2m 53s the patch passed +1 mvneclipse 0m 42s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 53s the patch passed +1 javadoc 2m 4s the patch passed -1 unit 7m 45s hadoop-common in the patch failed. +1 unit 0m 58s hadoop-hdfs-client in the patch passed. -1 unit 76m 55s hadoop-hdfs in the patch failed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 138m 17s Reason Tests Failed junit tests hadoop.fs.viewfs.TestViewFsTrash   hadoop.fs.TestTrash   hadoop.hdfs.TestDFSPermission   hadoop.hdfs.TestDFSShell   hadoop.hdfs.TestHDFSTrash   hadoop.hdfs.server.namenode.ha.TestHAAppend   hadoop.hdfs.server.namenode.TestDecommissioningStatus   hadoop.hdfs.TestEncryptionZones   hadoop.hdfs.TestEncryptionZonesWithKMS Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823813/HDFS-8312-001.patch JIRA Issue HDFS-8312 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux c12a826255c2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 02abd13 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16433/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16433/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16433/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16433/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 1m 51s Maven dependency ordering for branch
          +1 mvninstall 7m 16s trunk passed
          +1 compile 7m 20s trunk passed
          +1 checkstyle 1m 30s trunk passed
          +1 mvnsite 2m 33s trunk passed
          +1 mvneclipse 0m 39s trunk passed
          +1 findbugs 4m 45s trunk passed
          +1 javadoc 2m 4s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 2m 1s the patch passed
          +1 compile 7m 16s the patch passed
          +1 cc 7m 15s the patch passed
          +1 javac 7m 15s the patch passed
          +1 checkstyle 1m 28s the patch passed
          +1 mvnsite 2m 27s the patch passed
          +1 mvneclipse 0m 38s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 1s the patch passed
          +1 javadoc 2m 5s the patch passed
          +1 unit 7m 28s hadoop-common in the patch passed.
          +1 unit 0m 58s hadoop-hdfs-client in the patch passed.
          -1 unit 58m 15s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          117m 27s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.ha.TestBootstrapStandby
            hadoop.hdfs.TestDFSPermission
            hadoop.hdfs.server.datanode.fsdataset.impl.TestLazyPersistReplicaRecovery



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823828/HDFS-8312-002.patch
          JIRA Issue HDFS-8312
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux 2b51fd963b20 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / b8a446b
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16434/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16434/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16434/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 1m 51s Maven dependency ordering for branch +1 mvninstall 7m 16s trunk passed +1 compile 7m 20s trunk passed +1 checkstyle 1m 30s trunk passed +1 mvnsite 2m 33s trunk passed +1 mvneclipse 0m 39s trunk passed +1 findbugs 4m 45s trunk passed +1 javadoc 2m 4s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 2m 1s the patch passed +1 compile 7m 16s the patch passed +1 cc 7m 15s the patch passed +1 javac 7m 15s the patch passed +1 checkstyle 1m 28s the patch passed +1 mvnsite 2m 27s the patch passed +1 mvneclipse 0m 38s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 1s the patch passed +1 javadoc 2m 5s the patch passed +1 unit 7m 28s hadoop-common in the patch passed. +1 unit 0m 58s hadoop-hdfs-client in the patch passed. -1 unit 58m 15s hadoop-hdfs in the patch failed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 117m 27s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.ha.TestBootstrapStandby   hadoop.hdfs.TestDFSPermission   hadoop.hdfs.server.datanode.fsdataset.impl.TestLazyPersistReplicaRecovery Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823828/HDFS-8312-002.patch JIRA Issue HDFS-8312 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux 2b51fd963b20 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b8a446b Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16434/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16434/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16434/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          cheersyang Weiwei Yang added a comment -

          Hello Eric Yang

          Can you help to review the patch? The 3 failed test case doesn't seem to be related, I can get them run successfully on my local environment. I'll try a closer look tomorrow and confirm if they are unrelated.

          The patch added an option TO_TRASH in Options.Rename, to indicate if it is going to rename things to trash directory. In Instead of calling

          fs.rename(path, trashPath);

          in TrashPolicyDefault, calls

          fs.rename(path, trashPath, Rename.TO_TRASH);

          If user removes things without skipping trash, instead of checking rename permission, it is going to check if delete is permitted. If it failed, move to trash will fail with a permission denied error to avoid user from removing files/dirs that it has no permission to.

          Thanks!

          Show
          cheersyang Weiwei Yang added a comment - Hello Eric Yang Can you help to review the patch? The 3 failed test case doesn't seem to be related, I can get them run successfully on my local environment. I'll try a closer look tomorrow and confirm if they are unrelated. The patch added an option TO_TRASH in Options.Rename , to indicate if it is going to rename things to trash directory. In Instead of calling fs.rename(path, trashPath); in TrashPolicyDefault , calls fs.rename(path, trashPath, Rename.TO_TRASH); If user removes things without skipping trash, instead of checking rename permission, it is going to check if delete is permitted. If it failed, move to trash will fail with a permission denied error to avoid user from removing files/dirs that it has no permission to. Thanks!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 1m 33s Maven dependency ordering for branch
          +1 mvninstall 7m 24s trunk passed
          +1 compile 6m 43s trunk passed
          +1 checkstyle 1m 26s trunk passed
          +1 mvnsite 2m 18s trunk passed
          +1 mvneclipse 0m 37s trunk passed
          +1 findbugs 4m 22s trunk passed
          +1 javadoc 1m 58s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 54s the patch passed
          +1 compile 6m 41s the patch passed
          +1 cc 6m 41s the patch passed
          +1 javac 6m 41s the patch passed
          +1 checkstyle 1m 23s the patch passed
          +1 mvnsite 2m 14s the patch passed
          +1 mvneclipse 0m 37s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 46s the patch passed
          +1 javadoc 1m 58s the patch passed
          +1 unit 7m 26s hadoop-common in the patch passed.
          +1 unit 0m 55s hadoop-hdfs-client in the patch passed.
          -1 unit 59m 40s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          115m 44s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSPermission
            hadoop.hdfs.server.namenode.ha.TestEditLogTailer
            hadoop.metrics2.sink.TestRollingFileSystemSinkWithSecureHdfs



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824091/HDFS-8312-003.patch
          JIRA Issue HDFS-8312
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux 1de60f807657 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 2353271
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16447/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16447/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16447/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 1m 33s Maven dependency ordering for branch +1 mvninstall 7m 24s trunk passed +1 compile 6m 43s trunk passed +1 checkstyle 1m 26s trunk passed +1 mvnsite 2m 18s trunk passed +1 mvneclipse 0m 37s trunk passed +1 findbugs 4m 22s trunk passed +1 javadoc 1m 58s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 54s the patch passed +1 compile 6m 41s the patch passed +1 cc 6m 41s the patch passed +1 javac 6m 41s the patch passed +1 checkstyle 1m 23s the patch passed +1 mvnsite 2m 14s the patch passed +1 mvneclipse 0m 37s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 46s the patch passed +1 javadoc 1m 58s the patch passed +1 unit 7m 26s hadoop-common in the patch passed. +1 unit 0m 55s hadoop-hdfs-client in the patch passed. -1 unit 59m 40s hadoop-hdfs in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 115m 44s Reason Tests Failed junit tests hadoop.hdfs.TestDFSPermission   hadoop.hdfs.server.namenode.ha.TestEditLogTailer   hadoop.metrics2.sink.TestRollingFileSystemSinkWithSecureHdfs Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824091/HDFS-8312-003.patch JIRA Issue HDFS-8312 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux 1de60f807657 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2353271 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16447/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16447/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16447/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          eyang Eric Yang added a comment -

          TestDFSPermission test case seems to be failing. WeiWei could you take a look? Thanks

          Show
          eyang Eric Yang added a comment - TestDFSPermission test case seems to be failing. WeiWei could you take a look? Thanks
          Hide
          cheersyang Weiwei Yang added a comment -

          You are correct, I just found that after I enable trash in testTrashPermission, it caused the failure in testPermissionSetting, as they share the fs instance. After I disable trash once my test case is done, all UT can pass now. This is not a code change issue, just a UT fix, I just uploaded a new patch to address this.

          Thanks!

          Show
          cheersyang Weiwei Yang added a comment - You are correct, I just found that after I enable trash in testTrashPermission , it caused the failure in testPermissionSetting , as they share the fs instance. After I disable trash once my test case is done, all UT can pass now. This is not a code change issue, just a UT fix, I just uploaded a new patch to address this. Thanks!
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 1m 47s Maven dependency ordering for branch
          +1 mvninstall 6m 52s trunk passed
          +1 compile 6m 53s trunk passed
          +1 checkstyle 1m 27s trunk passed
          +1 mvnsite 2m 20s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 4m 27s trunk passed
          +1 javadoc 2m 1s trunk passed
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 2m 8s the patch passed
          +1 compile 7m 24s the patch passed
          +1 cc 7m 24s the patch passed
          +1 javac 7m 24s the patch passed
          -0 checkstyle 1m 31s root: The patch generated 1 new + 193 unchanged - 0 fixed = 194 total (was 193)
          +1 mvnsite 2m 28s the patch passed
          +1 mvneclipse 0m 38s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 8s the patch passed
          +1 javadoc 2m 5s the patch passed
          +1 unit 7m 33s hadoop-common in the patch passed.
          +1 unit 1m 1s hadoop-hdfs-client in the patch passed.
          +1 unit 60m 29s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          118m 41s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824284/HDFS-8312-004.patch
          JIRA Issue HDFS-8312
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux 67427cc15b7d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 20f0eb8
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16467/artifact/patchprocess/diff-checkstyle-root.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16467/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16467/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 1m 47s Maven dependency ordering for branch +1 mvninstall 6m 52s trunk passed +1 compile 6m 53s trunk passed +1 checkstyle 1m 27s trunk passed +1 mvnsite 2m 20s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 4m 27s trunk passed +1 javadoc 2m 1s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 2m 8s the patch passed +1 compile 7m 24s the patch passed +1 cc 7m 24s the patch passed +1 javac 7m 24s the patch passed -0 checkstyle 1m 31s root: The patch generated 1 new + 193 unchanged - 0 fixed = 194 total (was 193) +1 mvnsite 2m 28s the patch passed +1 mvneclipse 0m 38s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 8s the patch passed +1 javadoc 2m 5s the patch passed +1 unit 7m 33s hadoop-common in the patch passed. +1 unit 1m 1s hadoop-hdfs-client in the patch passed. +1 unit 60m 29s hadoop-hdfs in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 118m 41s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824284/HDFS-8312-004.patch JIRA Issue HDFS-8312 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux 67427cc15b7d 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 20f0eb8 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16467/artifact/patchprocess/diff-checkstyle-root.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16467/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16467/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 1m 34s Maven dependency ordering for branch
          +1 mvninstall 6m 38s trunk passed
          +1 compile 6m 43s trunk passed
          +1 checkstyle 1m 25s trunk passed
          +1 mvnsite 2m 20s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 4m 20s trunk passed
          +1 javadoc 2m 0s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 54s the patch passed
          +1 compile 6m 41s the patch passed
          +1 cc 6m 41s the patch passed
          +1 javac 6m 41s the patch passed
          +1 checkstyle 1m 25s the patch passed
          +1 mvnsite 2m 17s the patch passed
          +1 mvneclipse 0m 36s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 6s the patch passed
          +1 javadoc 2m 4s the patch passed
          -1 unit 7m 31s hadoop-common in the patch failed.
          +1 unit 0m 59s hadoop-hdfs-client in the patch passed.
          +1 unit 61m 16s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          117m 21s



          Reason Tests
          Failed junit tests hadoop.security.TestGroupsCaching



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824485/HDFS-8312-005.patch
          JIRA Issue HDFS-8312
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux 693e482656fb 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c5c3e81
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16477/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16477/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16477/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 1m 34s Maven dependency ordering for branch +1 mvninstall 6m 38s trunk passed +1 compile 6m 43s trunk passed +1 checkstyle 1m 25s trunk passed +1 mvnsite 2m 20s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 4m 20s trunk passed +1 javadoc 2m 0s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 54s the patch passed +1 compile 6m 41s the patch passed +1 cc 6m 41s the patch passed +1 javac 6m 41s the patch passed +1 checkstyle 1m 25s the patch passed +1 mvnsite 2m 17s the patch passed +1 mvneclipse 0m 36s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 6s the patch passed +1 javadoc 2m 4s the patch passed -1 unit 7m 31s hadoop-common in the patch failed. +1 unit 0m 59s hadoop-hdfs-client in the patch passed. +1 unit 61m 16s hadoop-hdfs in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 117m 21s Reason Tests Failed junit tests hadoop.security.TestGroupsCaching Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824485/HDFS-8312-005.patch JIRA Issue HDFS-8312 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux 693e482656fb 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c5c3e81 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16477/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16477/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16477/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          cheersyang Weiwei Yang added a comment -

          Hello Eric Yang

          Can you help to review latest v5 patch? The test failure TestGroupsCaching was not related, there is an opening JIRA HADOOP-13375 to track this, and you can see the prior result shows all clean.

          Show
          cheersyang Weiwei Yang added a comment - Hello Eric Yang Can you help to review latest v5 patch? The test failure TestGroupsCaching was not related, there is an opening JIRA HADOOP-13375 to track this, and you can see the prior result shows all clean.
          Hide
          eyang Eric Yang added a comment -

          +1 looks good. I just committed this to branch-2 and trunk. Thank you, Weiwei.

          Show
          eyang Eric Yang added a comment - +1 looks good. I just committed this to branch-2 and trunk. Thank you, Weiwei.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10323 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10323/)
          HDFS-8312. Added permission check for moving file to Trash. (Weiwei Yang (eyang: rev c49333becfa7652460976a61eb86522010bcfeed)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/ClientNamenodeProtocol.proto
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSPermission.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Options.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10323 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10323/ ) HDFS-8312 . Added permission check for moving file to Trash. (Weiwei Yang (eyang: rev c49333becfa7652460976a61eb86522010bcfeed) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/ClientNamenodeProtocol.proto (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSPermission.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Options.java
          Hide
          vinayrpet Vinayakumar B added a comment -

          I think this issue is critical, can be merged to branch-2.8 and 2.7 also.

          Show
          vinayrpet Vinayakumar B added a comment - I think this issue is critical, can be merged to branch-2.8 and 2.7 also.
          Hide
          daryn Daryn Sharp added a comment -

          Encountered a minor conflict with this patch so I took a look. If you view this as a security hole, then adding a new flag to rename and expecting well behaved clients to pass the flag is meaningless. I'll just use the original rename to toss your directory in the trash.

          NN-side trash support is a terrible idea. The NN should not be deleting directories as the super-user on behalf of non-priviledged users. The general solution to support trash policies using various directories isn't straightforward.

          Show
          daryn Daryn Sharp added a comment - Encountered a minor conflict with this patch so I took a look. If you view this as a security hole, then adding a new flag to rename and expecting well behaved clients to pass the flag is meaningless. I'll just use the original rename to toss your directory in the trash. NN-side trash support is a terrible idea. The NN should not be deleting directories as the super-user on behalf of non-priviledged users. The general solution to support trash policies using various directories isn't straightforward.
          Hide
          cheersyang Weiwei Yang added a comment -

          Hello Daryn Sharp

          The NN should not be deleting directories as the super-user on behalf of non-privileged users.

          I agree with that. But the permission should be checked before moving the files/dirs to trash. If you use the original rename to move the files/dirs to trash, after a while, deletion is denied due to insufficient permission, that looks like odd.

          Show
          cheersyang Weiwei Yang added a comment - Hello Daryn Sharp The NN should not be deleting directories as the super-user on behalf of non-privileged users. I agree with that. But the permission should be checked before moving the files/dirs to trash. If you use the original rename to move the files/dirs to trash, after a while, deletion is denied due to insufficient permission, that looks like odd.
          Hide
          daryn Daryn Sharp added a comment -

          If you use the original rename to move the files/dirs to trash, after a while, deletion is denied due to insufficient permission, that looks like odd.

          A user always has permissions to their own trash. When would insufficient permissions occur?

          Show
          daryn Daryn Sharp added a comment - If you use the original rename to move the files/dirs to trash, after a while, deletion is denied due to insufficient permission, that looks like odd. A user always has permissions to their own trash. When would insufficient permissions occur?
          Hide
          cheersyang Weiwei Yang added a comment -

          Hello Daryn Sharp

          A user always has permissions to their own trash

          True

          When would insufficient permissions occur?

          Suppose userA has no privilege to delete fileB, directly FS.delete(fileB) will fail. However FS.rename(fileB, fileBinTrash) would success because it only checks the write access to parent of fileB and write access to ancestor of fileBinTrash. You can also refer the reproduce steps in description and this comment .

          The patch fixed the rename API by adding the permission check of delete when the destination is to the trash directory, this has to be fixed otherwise the it exposes the security hole that malicious user would use rename to move other people's file/dir to trash and subsequently got deleted.

          Show
          cheersyang Weiwei Yang added a comment - Hello Daryn Sharp A user always has permissions to their own trash True When would insufficient permissions occur? Suppose userA has no privilege to delete fileB, directly FS.delete(fileB) will fail. However FS.rename(fileB, fileBinTrash) would success because it only checks the write access to parent of fileB and write access to ancestor of fileBinTrash. You can also refer the reproduce steps in description and this comment . The patch fixed the rename API by adding the permission check of delete when the destination is to the trash directory, this has to be fixed otherwise the it exposes the security hole that malicious user would use rename to move other people's file/dir to trash and subsequently got deleted.
          Hide
          cheersyang Weiwei Yang added a comment -

          Per discussion in HADOOP-10922 here, propose to back port this to branch-2.8. I will create a patch for 2.8 shortly. Does that make sense?

          Show
          cheersyang Weiwei Yang added a comment - Per discussion in HADOOP-10922 here , propose to back port this to branch-2.8. I will create a patch for 2.8 shortly. Does that make sense?
          Hide
          brahmareddy Brahma Reddy Battula added a comment -

          Uploading the branch-2.7 patch.Kindly Review.

          Show
          brahmareddy Brahma Reddy Battula added a comment - Uploading the branch-2.7 patch.Kindly Review.
          Hide
          shv Konstantin Shvachko added a comment -

          +1 the backport for branch-2.7 looks good.

          Show
          shv Konstantin Shvachko added a comment - +1 the backport for branch-2.7 looks good.
          Hide
          brahmareddy Brahma Reddy Battula added a comment -

          Weiwei Yang do you have plans update patch to branch-2.8..?

          Show
          brahmareddy Brahma Reddy Battula added a comment - Weiwei Yang do you have plans update patch to branch-2.8..?
          Hide
          cheersyang Weiwei Yang added a comment -

          Hi Brahma Reddy Battula sure, I created HDFS-11829 for this backport and linked it as related to this one. Lets track the work there instead of reopening this one, does that look good to you?

          Show
          cheersyang Weiwei Yang added a comment - Hi Brahma Reddy Battula sure, I created HDFS-11829 for this backport and linked it as related to this one. Lets track the work there instead of reopening this one, does that look good to you?
          Hide
          shv Konstantin Shvachko added a comment -

          Weiwei Yang you could have re-used HDFS-11784 for both backports as it is still open. But It works that way too.
          +1 for backport to 2.8.

          Show
          shv Konstantin Shvachko added a comment - Weiwei Yang you could have re-used HDFS-11784 for both backports as it is still open. But It works that way too. +1 for backport to 2.8.
          Hide
          brahmareddy Brahma Reddy Battula added a comment -

          Lets track the work there instead of reopening this one, does that look good to you?

          you could have attached in HDFS-11784 which is raised for branch-2.7 (could have changed the summary.), anyway it's should be ok.

          Show
          brahmareddy Brahma Reddy Battula added a comment - Lets track the work there instead of reopening this one, does that look good to you? you could have attached in HDFS-11784 which is raised for branch-2.7 (could have changed the summary.), anyway it's should be ok.
          Hide
          brahmareddy Brahma Reddy Battula added a comment -

          Updated the fix versions once after committed to branch-2.7 and branch-2.8.1.

          Show
          brahmareddy Brahma Reddy Battula added a comment - Updated the fix versions once after committed to branch-2.7 and branch-2.8.1 .
          Hide
          daryn Daryn Sharp added a comment -

          The patch fixed the rename API by adding the permission check of delete when the destination is to the trash directory, this has to be fixed otherwise the it exposes the security hole that malicious user would use rename to move other people's file/dir to trash and subsequently got deleted.

          I lost track of this jira until I saw it being backported. I'll reiterate, bluntly this time, that this patch is completely worthless from a security perspective. It's an honor-system based sanity check for the good users. A malicious user is never going to pass the flag to request the permission subcheck. Why even hack fs -rm to remove the flag when you can just use fs -mv?

          Suppose userA has no privilege to delete fileB, directly FS.delete(fileB) will fail. However FS.rename(fileB, fileBinTrash) would success because it only checks the write access to parent of fileB and write access to ancestor of fileBinTrash.

          Yes, rename/delete modify a directory which only requires write privs. That's POSIX semantics. Small corrections, assuming user has write privs to a specific dir:

          1. delete(fileB) does and should succeed regardless of fileB permissions – ignoring sticky bit rules for simplicity
          2. delete(dirB) will fail if dirB is non-empty and the user has no permission. the user has to descend the tree (read privs), and remove the children (write privs)
          3. rename always works on a file or subdir regardless of the permission on either.

          ––

          Consider a *nix system. Let's say I foolishly have a single volume for the entire system, and I run tmpwatch to delete old stuff in /tmp. It's the same situation. If I have write privs to a directory, I can move anything in it to /tmp and it'll get blown away.

          Show
          daryn Daryn Sharp added a comment - The patch fixed the rename API by adding the permission check of delete when the destination is to the trash directory, this has to be fixed otherwise the it exposes the security hole that malicious user would use rename to move other people's file/dir to trash and subsequently got deleted. I lost track of this jira until I saw it being backported. I'll reiterate, bluntly this time, that this patch is completely worthless from a security perspective . It's an honor-system based sanity check for the good users. A malicious user is never going to pass the flag to request the permission subcheck. Why even hack fs -rm to remove the flag when you can just use fs -mv? Suppose userA has no privilege to delete fileB, directly FS.delete(fileB) will fail. However FS.rename(fileB, fileBinTrash) would success because it only checks the write access to parent of fileB and write access to ancestor of fileBinTrash. Yes, rename/delete modify a directory which only requires write privs. That's POSIX semantics. Small corrections, assuming user has write privs to a specific dir: delete(fileB) does and should succeed regardless of fileB permissions – ignoring sticky bit rules for simplicity delete(dirB) will fail if dirB is non-empty and the user has no permission. the user has to descend the tree (read privs), and remove the children (write privs) rename always works on a file or subdir regardless of the permission on either. –– Consider a *nix system. Let's say I foolishly have a single volume for the entire system, and I run tmpwatch to delete old stuff in /tmp. It's the same situation. If I have write privs to a directory, I can move anything in it to /tmp and it'll get blown away.
          Hide
          eyang Eric Yang added a comment -

          Daryn Sharp The current implementation is only good for protecting common mistakes. The hacker proof implementation needs to come from server side trash rather than client API. This patch does not make HDFS client any worse than existing implementation. I will take 5% improvement today than waiting for the server side trash to come.

          Show
          eyang Eric Yang added a comment - Daryn Sharp The current implementation is only good for protecting common mistakes. The hacker proof implementation needs to come from server side trash rather than client API. This patch does not make HDFS client any worse than existing implementation. I will take 5% improvement today than waiting for the server side trash to come.
          Hide
          cheersyang Weiwei Yang added a comment -

          Hi Daryn Sharp

          Thank you for sharing your thoughts, I appreciate that.

          It's an honor-system based sanity check for the good users. A malicious user is never going to pass the flag to request the permission subcheck. Why even hack fs -rm to remove the flag when you can just use fs -mv?

          This fix is only fixed the internal code in fs, not exposed this flag to any clients. Because when trash is enabled, when user runs

          hdfs dfs -rm /path/to/file

          at backend, it is implemented by rename which only checks the privilege to rename. This behavior changes semantic under-cover which caused inconsistent behavior with or without -skipTrash flag, isn't it? The purpose of the fix is to prevent user to remove files which they don't have privilege to. More explanation is here .

          Consider a *nix system. Let's say I foolishly have a single volume for the entire system, and I run tmpwatch to delete old stuff in /tmp. It's the same situation. If I have write privs to a directory, I can move anything in it to /tmp and it'll get blown away.

          This is different, user uses mv to /tmp instead of what trash uses rm, so they know what they are doing and it still honors mv semantic. On the contrary, moving stuff to trash is using rm so if it doesn't check rm privilege seems inconsistent to me.

          Please share your ideas if you agree this is a problem that we need to fix. In that case, if you have a better idea how to fix this, I would love to hear.

          Thank you.

          Show
          cheersyang Weiwei Yang added a comment - Hi Daryn Sharp Thank you for sharing your thoughts, I appreciate that. It's an honor-system based sanity check for the good users. A malicious user is never going to pass the flag to request the permission subcheck. Why even hack fs -rm to remove the flag when you can just use fs -mv? This fix is only fixed the internal code in fs, not exposed this flag to any clients. Because when trash is enabled, when user runs hdfs dfs -rm /path/to/file at backend, it is implemented by rename which only checks the privilege to rename . This behavior changes semantic under-cover which caused inconsistent behavior with or without -skipTrash flag, isn't it? The purpose of the fix is to prevent user to remove files which they don't have privilege to. More explanation is here . Consider a *nix system. Let's say I foolishly have a single volume for the entire system, and I run tmpwatch to delete old stuff in /tmp. It's the same situation. If I have write privs to a directory, I can move anything in it to /tmp and it'll get blown away. This is different, user uses mv to /tmp instead of what trash uses rm , so they know what they are doing and it still honors mv semantic. On the contrary, moving stuff to trash is using rm so if it doesn't check rm privilege seems inconsistent to me. Please share your ideas if you agree this is a problem that we need to fix. In that case, if you have a better idea how to fix this, I would love to hear. Thank you.

            People

            • Assignee:
              cheersyang Weiwei Yang
              Reporter:
              eyang Eric Yang
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development