From b395f58e850e70bcb9273d86510276ce28962a85 Mon Sep 17 00:00:00 2001 From: Haohui Mai Date: Fri, 21 Nov 2014 11:01:14 -0800 Subject: [PATCH] HDFS-7420. Delegate permission checks to FSDirectory. Contributed by Haohui Mai. --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../hdfs/server/namenode/FSDirectory.java | 84 ++++++++++++++++++- .../hdfs/server/namenode/FSNamesystem.java | 37 ++------ .../namenode/TestFSPermissionChecker.java | 2 +- 4 files changed, 95 insertions(+), 30 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 6a180b2f29e..865d6bd1a61 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -126,6 +126,8 @@ Release 2.7.0 - UNRELEASED HDFS-7415. Move FSNameSystem.resolvePath() to FSDirectory. (wheat9) + HDFS-7420. Delegate permission checks to FSDirectory. (wheat9) + OPTIMIZATIONS BUG FIXES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java index 929968d0684..5187fcf09fb 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java @@ -52,6 +52,7 @@ import org.apache.hadoop.fs.XAttr; import org.apache.hadoop.fs.XAttrSetFlag; import org.apache.hadoop.fs.permission.AclEntry; import org.apache.hadoop.fs.permission.AclStatus; +import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.fs.permission.PermissionStatus; import org.apache.hadoop.hdfs.DFSConfigKeys; @@ -96,6 +97,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import org.apache.hadoop.security.AccessControlException; +import org.apache.hadoop.security.UserGroupInformation; /** * Both FSDirectory and FSNamesystem manage the state of the namespace. @@ -152,6 +154,8 @@ public class FSDirectory implements Closeable { private final ReentrantReadWriteLock dirLock; private final boolean isPermissionEnabled; + private final String fsOwnerShortUserName; + private final String supergroup; // utility methods to acquire and release read lock and write lock void readLock() { @@ -195,13 +199,19 @@ public class FSDirectory implements Closeable { */ private final NameCache nameCache; - FSDirectory(FSNamesystem ns, Configuration conf) { + FSDirectory(FSNamesystem ns, Configuration conf) throws IOException { this.dirLock = new ReentrantReadWriteLock(true); // fair rootDir = createRoot(ns); inodeMap = INodeMap.newInstance(rootDir); this.isPermissionEnabled = conf.getBoolean( DFSConfigKeys.DFS_PERMISSIONS_ENABLED_KEY, DFSConfigKeys.DFS_PERMISSIONS_ENABLED_DEFAULT); + this.fsOwnerShortUserName = + UserGroupInformation.getCurrentUser().getShortUserName(); + this.supergroup = conf.get( + DFSConfigKeys.DFS_PERMISSIONS_SUPERUSERGROUP_KEY, + DFSConfigKeys.DFS_PERMISSIONS_SUPERUSERGROUP_DEFAULT); + int configuredLimit = conf.getInt( DFSConfigKeys.DFS_LIST_LIMIT, DFSConfigKeys.DFS_LIST_LIMIT_DEFAULT); this.lsLimit = configuredLimit>0 ? @@ -3333,4 +3343,76 @@ public class FSDirectory implements Closeable { } return inodesInPath; } + + FSPermissionChecker getPermissionChecker() + throws AccessControlException { + try { + return new FSPermissionChecker(fsOwnerShortUserName, supergroup, + NameNode.getRemoteUser()); + } catch (IOException ioe) { + throw new AccessControlException(ioe); + } + } + + void checkOwner(FSPermissionChecker pc, String path) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, true, null, null, null, null); + } + + void checkPathAccess(FSPermissionChecker pc, String path, + FsAction access) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, false, null, null, access, null); + } + void checkParentAccess( + FSPermissionChecker pc, String path, FsAction access) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, false, null, access, null, null); + } + + void checkAncestorAccess( + FSPermissionChecker pc, String path, FsAction access) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, false, access, null, null, null); + } + + void checkTraverse(FSPermissionChecker pc, String path) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, false, null, null, null, null); + } + + /** + * Check whether current user have permissions to access the path. For more + * details of the parameters, see + * {@link FSPermissionChecker#checkPermission}. + */ + private void checkPermission( + FSPermissionChecker pc, String path, boolean doCheckOwner, + FsAction ancestorAccess, FsAction parentAccess, FsAction access, + FsAction subAccess) + throws AccessControlException, UnresolvedLinkException { + checkPermission(pc, path, doCheckOwner, ancestorAccess, + parentAccess, access, subAccess, false, true); + } + + /** + * Check whether current user have permissions to access the path. For more + * details of the parameters, see + * {@link FSPermissionChecker#checkPermission}. + */ + void checkPermission( + FSPermissionChecker pc, String path, boolean doCheckOwner, + FsAction ancestorAccess, FsAction parentAccess, FsAction access, + FsAction subAccess, boolean ignoreEmptyDir, boolean resolveLink) + throws AccessControlException, UnresolvedLinkException { + if (!pc.isSuperUser()) { + readLock(); + try { + pc.checkPermission(path, this, doCheckOwner, ancestorAccess, + parentAccess, access, subAccess, ignoreEmptyDir, resolveLink); + } finally { + readUnlock(); + } + } + } } 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 9df29839f27..41e07acaee7 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 @@ -392,7 +392,6 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, static int BLOCK_DELETION_INCREMENT = 1000; private final boolean isPermissionEnabled; private final UserGroupInformation fsOwner; - private final String fsOwnerShortUserName; private final String supergroup; private final boolean standbyShouldCheckpoint; @@ -766,7 +765,6 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, DFS_STORAGE_POLICY_ENABLED_DEFAULT); this.fsOwner = UserGroupInformation.getCurrentUser(); - this.fsOwnerShortUserName = fsOwner.getShortUserName(); this.supergroup = conf.get(DFS_PERMISSIONS_SUPERUSERGROUP_KEY, DFS_PERMISSIONS_SUPERUSERGROUP_DEFAULT); this.isPermissionEnabled = conf.getBoolean(DFS_PERMISSIONS_ENABLED_KEY, @@ -3922,11 +3920,7 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, private FSPermissionChecker getPermissionChecker() throws AccessControlException { - try { - return new FSPermissionChecker(fsOwnerShortUserName, supergroup, getRemoteUser()); - } catch (IOException ioe) { - throw new AccessControlException(ioe); - } + return dir.getPermissionChecker(); } /** @@ -6411,13 +6405,13 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, private void checkOwner(FSPermissionChecker pc, String path) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, true, null, null, null, null); + dir.checkOwner(pc, path); } private void checkPathAccess(FSPermissionChecker pc, String path, FsAction access) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, false, null, null, access, null); + dir.checkPathAccess(pc, path, access); } private void checkUnreadableBySuperuser(FSPermissionChecker pc, @@ -6438,18 +6432,18 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, private void checkParentAccess(FSPermissionChecker pc, String path, FsAction access) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, false, null, access, null, null); + dir.checkParentAccess(pc, path, access); } private void checkAncestorAccess(FSPermissionChecker pc, String path, FsAction access) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, false, access, null, null, null); + dir.checkAncestorAccess(pc, path, access); } private void checkTraverse(FSPermissionChecker pc, String path) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, false, null, null, null, null); + dir.checkTraverse(pc, path); } @Override @@ -6470,30 +6464,17 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, String path, boolean doCheckOwner, FsAction ancestorAccess, FsAction parentAccess, FsAction access, FsAction subAccess) throws AccessControlException, UnresolvedLinkException { - checkPermission(pc, path, doCheckOwner, ancestorAccess, + checkPermission(pc, path, doCheckOwner, ancestorAccess, parentAccess, access, subAccess, false, true); } - /** - * Check whether current user have permissions to access the path. For more - * details of the parameters, see - * {@link FSPermissionChecker#checkPermission}. - */ private void checkPermission(FSPermissionChecker pc, String path, boolean doCheckOwner, FsAction ancestorAccess, FsAction parentAccess, FsAction access, FsAction subAccess, boolean ignoreEmptyDir, boolean resolveLink) throws AccessControlException, UnresolvedLinkException { - if (!pc.isSuperUser()) { - waitForLoadingFSImage(); - readLock(); - try { - pc.checkPermission(path, dir, doCheckOwner, ancestorAccess, - parentAccess, access, subAccess, ignoreEmptyDir, resolveLink); - } finally { - readUnlock(); - } - } + dir.checkPermission(pc, path, doCheckOwner, ancestorAccess, parentAccess, + access, subAccess, ignoreEmptyDir, resolveLink); } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSPermissionChecker.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSPermissionChecker.java index c2ca9838c91..4ae8d2663ea 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSPermissionChecker.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSPermissionChecker.java @@ -75,7 +75,7 @@ public class TestFSPermissionChecker { private INodeDirectory inodeRoot; @Before - public void setUp() { + public void setUp() throws IOException { Configuration conf = new Configuration(); FSNamesystem fsn = mock(FSNamesystem.class); doAnswer(new Answer() {