HDFS-6556. Refine XAttr permissions. Contributed by Uma Maheswara Rao G.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1606320 13f79535-47bb-0310-9956-ffa450edef68

hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

@@ -728,6 +728,8 @@ Release 2.5.0 - UNRELEASED
     HADOOP-10701. NFS should not validate the access premission only based on
     the user's primary group (Harsh J via atm)
 
+    HDFS-6556. Refine XAttr permissions (umamahesh)
+
   BREAKDOWN OF HDFS-2006 SUBTASKS AND RELATED JIRAS
 
     HDFS-6299. Protobuf for XAttr and client-side implementation. (Yi Liu via umamahesh)

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/XAttr.java

@@ -30,11 +30,11 @@ import org.apache.hadoop.classification.InterfaceAudience;
  * namespaces are defined: user, trusted, security and system.
  *   1) USER namespace attributes may be used by any user to store
  *   arbitrary information. Access permissions in this namespace are
- *   defined by a file directory's permission bits.
+ *   defined by a file directory's permission bits. For sticky directories,
+ *   only the owner and privileged user can write attributes.
  * <br>
  *   2) TRUSTED namespace attributes are only visible and accessible to
- *   privileged users (a file or directory's owner or the fs
- *   admin). This namespace is available from both user space
+ *   privileged users. This namespace is available from both user space
  *   (filesystem API) and fs kernel.
  * <br>
  *   3) SYSTEM namespace attributes are used by the fs kernel to store

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

@@ -8196,10 +8196,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats,
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot set XAttr on " + src);
       src = FSDirectory.resolvePath(src, pathComponents, dir);
-      if (isPermissionEnabled) {
-        checkOwner(pc, src);
-        checkPathAccess(pc, src, FsAction.WRITE);
-      }
+      checkXAttrChangeAccess(src, xAttr, pc);
       List<XAttr> xAttrs = Lists.newArrayListWithCapacity(1);
       xAttrs.add(xAttr);
       dir.setXAttrs(src, xAttrs, flag);
@@ -8319,10 +8316,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats,
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot remove XAttr entry on " + src);
       src = FSDirectory.resolvePath(src, pathComponents, dir);
-      if (isPermissionEnabled) {
-        checkOwner(pc, src);
-        checkPathAccess(pc, src, FsAction.WRITE);
-      }
+      checkXAttrChangeAccess(src, xAttr, pc);
 
       List<XAttr> xAttrs = Lists.newArrayListWithCapacity(1);
       xAttrs.add(xAttr);
@@ -8341,6 +8335,21 @@ public class FSNamesystem implements Namesystem, FSClusterStats,
     logAuditEvent(true, "removeXAttr", src, null, resultingStat);
   }
 
+  private void checkXAttrChangeAccess(String src, XAttr xAttr,
+      FSPermissionChecker pc) throws UnresolvedLinkException,
+      AccessControlException {
+    if (isPermissionEnabled && xAttr.getNameSpace() == XAttr.NameSpace.USER) {
+      final INode inode = dir.getINode(src);
+      if (inode.isDirectory() && inode.getFsPermission().getStickyBit()) {
+        if (!pc.isSuperUser()) {
+          checkOwner(pc, src);
+        }
+      } else {
+        checkPathAccess(pc, src, FsAction.WRITE);
+      }
+    }
+  }
+
   /**
    * Default AuditLogger implementation; used when no access logger is
    * defined in the config file. It can also be explicitly listed in the

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java

@@ -34,7 +34,8 @@ import com.google.common.collect.Lists;
  * USER - extended user attributes: these can be assigned to files and
  * directories to store arbitrary additional information. The access
  * permissions for user attributes are defined by the file permission
- * bits.
+ * bits. For sticky directories, only the owner and privileged user can 
+ * write attributes.
  * <br>
  * TRUSTED - trusted extended attributes: these are visible/accessible
  * only to/by the super user.

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java

@@ -2456,38 +2456,39 @@ public class TestDFSShell {
   }
 
   /**
-   * HDFS-6374 setXAttr should require the user to be the owner of the file
-   * or directory.
-   *
-   * Test to make sure that only the owner of a file or directory can set
-   * or remove the xattrs.
-   *
-   * As user1:
-   * Create a directory (/foo) as user1, chown it to user1 (and user1's group),
-   * grant rwx to "other".
-   *
-   * As user2:
-   * Set an xattr (should fail).
-   *
-   * As user1:
-   * Set an xattr (should pass).
-   *
-   * As user2:
-   * Read the xattr (should pass).
-   * Remove the xattr (should fail).
-   *
-   * As user1:
-   * Read the xattr (should pass).
-   * Remove the xattr (should pass).
+   * 
+   * Test to make sure that user namespace xattrs can be set only if path has
+   * access and for sticky directorries, only owner/privileged user can write.
+   * Trusted namespace xattrs can be set only with privileged users.
+   * 
+   * As user1: Create a directory (/foo) as user1, chown it to user1 (and
+   * user1's group), grant rwx to "other".
+   * 
+   * As user2: Set an xattr (should pass with path access).
+   * 
+   * As user1: Set an xattr (should pass).
+   * 
+   * As user2: Read the xattr (should pass). Remove the xattr (should pass with
+   * path access).
+   * 
+   * As user1: Read the xattr (should pass). Remove the xattr (should pass).
+   * 
+   * As user1: Change permissions only to owner
+   * 
+   * As User2: Set an Xattr (Should fail set with no path access) Remove an
+   * Xattr (Should fail with no path access)
+   * 
+   * As SuperUser: Set an Xattr with Trusted (Should pass)
    */
   @Test (timeout = 30000)
   public void testSetXAttrPermissionAsDifferentOwner() throws Exception {
     final String USER1 = "user1";
-    final String GROUP1 = "mygroup1";
+    final String GROUP1 = "supergroup";
     final UserGroupInformation user1 = UserGroupInformation.
         createUserForTesting(USER1, new String[] {GROUP1});
     final UserGroupInformation user2 = UserGroupInformation.
         createUserForTesting("user2", new String[] {"mygroup2"});
+    final UserGroupInformation SUPERUSER = UserGroupInformation.getCurrentUser();
     MiniDFSCluster cluster = null;
     PrintStream bak = null;
     try {
@@ -2503,7 +2504,7 @@ public class TestDFSShell {
       final ByteArrayOutputStream out = new ByteArrayOutputStream();
       System.setErr(new PrintStream(out));
 
-      // mkdir foo as user1
+      //Test 1.  Let user1 be owner for /foo
       user1.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
@@ -2514,7 +2515,8 @@ public class TestDFSShell {
           return null;
         }
       });
-
+      
+      //Test 2. Give access to others
       user1.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
@@ -2527,23 +2529,21 @@ public class TestDFSShell {
         }
       });
 
-      // No permission to write xattr for non-owning user (user2).
+      // Test 3. Should be allowed to write xattr if there is a path access to
+      // user (user2).
       user2.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
           final int ret = ToolRunner.run(fshell, new String[]{
               "-setfattr", "-n", "user.a1", "-v", "1234", "/foo"});
-          assertEquals("Returned should be 1", 1, ret);
-          final String str = out.toString();
-          assertTrue("Permission denied printed",
-              str.indexOf("Permission denied") != -1);
+          assertEquals("Returned should be 0", 0, ret);
           out.reset();
           return null;
         }
       });
 
-      // But there should be permission to write xattr for
-      // the owning user.
+      //Test 4. There should be permission to write xattr for
+      // the owning user with write permissions.
       user1.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
@@ -2555,19 +2555,55 @@ public class TestDFSShell {
         }
       });
 
-      // There should be permission to read,but not to remove for
-      // non-owning user (user2).
+      // Test 5. There should be permission to read non-owning user (user2) if
+      // there is path access to that user and also can remove.
       user2.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
           // Read
-          int ret = ToolRunner.run(fshell, new String[]{
-              "-getfattr", "-n", "user.a1", "/foo"});
+          int ret = ToolRunner.run(fshell, new String[] { "-getfattr", "-n",
+              "user.a1", "/foo" });
           assertEquals("Returned should be 0", 0, ret);
           out.reset();
           // Remove
-          ret = ToolRunner.run(fshell, new String[]{
-              "-setfattr", "-x", "user.a1", "/foo"});
+          ret = ToolRunner.run(fshell, new String[] { "-setfattr", "-x",
+              "user.a1", "/foo" });
+          assertEquals("Returned should be 0", 0, ret);
+          out.reset();
+          return null;
+        }
+      });
+
+      // Test 6. There should be permission to read/remove for
+      // the owning user with path access.
+      user1.doAs(new PrivilegedExceptionAction<Object>() {
+        @Override
+        public Object run() throws Exception {
+          return null;
+        }
+      });
+      
+      // Test 7. Change permission to have path access only to owner(user1)
+      user1.doAs(new PrivilegedExceptionAction<Object>() {
+        @Override
+        public Object run() throws Exception {
+          // Give access to "other"
+          final int ret = ToolRunner.run(fshell, new String[]{
+              "-chmod", "700", "/foo"});
+          assertEquals("Return should be 0", 0, ret);
+          out.reset();
+          return null;
+        }
+      });
+      
+      // Test 8. There should be no permissions to set for
+      // the non-owning user with no path access.
+      user2.doAs(new PrivilegedExceptionAction<Object>() {
+        @Override
+        public Object run() throws Exception {
+          // set
+          int ret = ToolRunner.run(fshell, new String[] { "-setfattr", "-n",
+              "user.a2", "/foo" });
           assertEquals("Returned should be 1", 1, ret);
           final String str = out.toString();
           assertTrue("Permission denied printed",
@@ -2576,20 +2612,31 @@ public class TestDFSShell {
           return null;
         }
       });
-
-      // But there should be permission to read/remove for
-      // the owning user.
-      user1.doAs(new PrivilegedExceptionAction<Object>() {
+      
+      // Test 9. There should be no permissions to remove for
+      // the non-owning user with no path access.
+      user2.doAs(new PrivilegedExceptionAction<Object>() {
         @Override
         public Object run() throws Exception {
-          // Read
-          int ret = ToolRunner.run(fshell, new String[]{
-              "-getfattr", "-n", "user.a1", "/foo"});
-          assertEquals("Returned should be 0", 0, ret);
+          // set
+          int ret = ToolRunner.run(fshell, new String[] { "-setfattr", "-x",
+              "user.a2", "/foo" });
+          assertEquals("Returned should be 1", 1, ret);
+          final String str = out.toString();
+          assertTrue("Permission denied printed",
+              str.indexOf("Permission denied") != -1);
           out.reset();
-          // Remove
-          ret = ToolRunner.run(fshell, new String[]{
-              "-setfattr", "-x", "user.a1", "/foo"});
+          return null;
+        }
+      });
+      
+      // Test 10. Superuser should be allowed to set with trusted namespace
+      SUPERUSER.doAs(new PrivilegedExceptionAction<Object>() {
+        @Override
+        public Object run() throws Exception {
+          // set
+          int ret = ToolRunner.run(fshell, new String[] { "-setfattr", "-n",
+              "trusted.a3", "/foo" });
           assertEquals("Returned should be 0", 0, ret);
           out.reset();
           return null;