diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 6302c290958..b5f30866d36 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -476,6 +476,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) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/XAttr.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/XAttr.java index 82272e22a31..99f629afdfe 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/XAttr.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/XAttr.java @@ -30,11 +30,11 @@ * 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. *
* 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. *
* 3) SYSTEM namespace attributes are used by the fs kernel to store diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index f929eaf6e37..278fbb2a0ab 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -8195,10 +8195,7 @@ private void setXAttrInt(String src, XAttr xAttr, EnumSet flag, 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 xAttrs = Lists.newArrayListWithCapacity(1); xAttrs.add(xAttr); dir.setXAttrs(src, xAttrs, flag); @@ -8318,10 +8315,7 @@ void removeXAttr(String src, XAttr xAttr) throws IOException { 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 xAttrs = Lists.newArrayListWithCapacity(1); xAttrs.add(xAttr); @@ -8340,6 +8334,21 @@ void removeXAttr(String src, XAttr xAttr) throws IOException { 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 diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java index 7fed362b9b2..47f29399e5a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java @@ -34,7 +34,8 @@ * 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. *
* TRUSTED - trusted extended attributes: these are visible/accessible * only to/by the super user. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java index 4a0704c8d49..d15ee01db66 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java @@ -2440,38 +2440,39 @@ private void doSetXattr(ByteArrayOutputStream out, FsShell fshell, } /** - * 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. + * 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 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 user2: Set an xattr (should pass with path access). * - * As user1: - * Set an xattr (should pass). + * As user1: Set an xattr (should pass). * - * As user2: - * Read the xattr (should pass). - * Remove the xattr (should fail). + * 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: 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 { @@ -2487,7 +2488,7 @@ public void testSetXAttrPermissionAsDifferentOwner() throws Exception { 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() { @Override public Object run() throws Exception { @@ -2499,6 +2500,7 @@ public Object run() throws Exception { } }); + //Test 2. Give access to others user1.doAs(new PrivilegedExceptionAction() { @Override public Object run() throws Exception { @@ -2511,23 +2513,21 @@ public Object run() throws Exception { } }); - // 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() { @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() { @Override public Object run() throws Exception { @@ -2539,19 +2539,55 @@ public Object run() throws Exception { } }); - // 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() { @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() { + @Override + public Object run() throws Exception { + return null; + } + }); + + // Test 7. Change permission to have path access only to owner(user1) + user1.doAs(new PrivilegedExceptionAction() { + @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() { + @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", @@ -2561,19 +2597,30 @@ public Object run() throws Exception { } }); - // But there should be permission to read/remove for - // the owning user. - user1.doAs(new PrivilegedExceptionAction() { + // Test 9. There should be no permissions to remove for + // the non-owning user with no path access. + user2.doAs(new PrivilegedExceptionAction() { @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() { + @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;