From e1d0953b99203e76e4e42f673e82d8bf59baed3b Mon Sep 17 00:00:00 2001 From: Thejas M Nair Date: Fri, 21 Apr 2017 02:07:31 -0700 Subject: [PATCH 1/2] HIVE-16497 : FileUtils. isActionPermittedForFileHierarchy, isOwnerOfFileHierarchy file system operations should be impersonated --- .../apache/hadoop/hive/common/FileUtils.java | 60 ++++++++++++++++--- .../plugin/sqlstd/SQLAuthorizationUtils.java | 4 +- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java index f401b6f2739f..f2f8f7fccad6 100644 --- a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java +++ b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java @@ -25,6 +25,8 @@ import java.net.URISyntaxException; import java.security.AccessControlException; import java.security.PrivilegedExceptionAction; +import java.util.ArrayList; +import java.util.Arrays; import java.util.BitSet; import java.util.Collection; import java.util.HashSet; @@ -352,6 +354,12 @@ public static FileStatus getPathOrParentThatExists(FileSystem fs, Path path) thr return getPathOrParentThatExists(fs, parentPath); } + public static void checkFileAccessWithImpersonation(final FileSystem fs, final FileStatus stat, + final FsAction action, final String user) + throws IOException, AccessControlException, InterruptedException, Exception { + checkFileAccessWithImpersonation(fs, stat, action, user, null); + } + /** * Perform a check to determine if the user is able to access the file passed in. * If the user name passed in is different from the current user, this method will @@ -366,13 +374,15 @@ public static FileStatus getPathOrParentThatExists(FileSystem fs, Path path) thr * check will be performed within a doAs() block to use the access privileges * of this user. In this case the user must be configured to impersonate other * users, otherwise this check will fail with error. + * @param children List of children to be collected. If this is null, no children are collected. + * To be set only if this is a directory * @throws IOException * @throws AccessControlException * @throws InterruptedException * @throws Exception */ public static void checkFileAccessWithImpersonation(final FileSystem fs, - final FileStatus stat, final FsAction action, final String user) + final FileStatus stat, final FsAction action, final String user, final List children) throws IOException, AccessControlException, InterruptedException, Exception { UserGroupInformation ugi = Utils.getUGI(); String currentUser = ugi.getShortUserName(); @@ -392,6 +402,15 @@ public static void checkFileAccessWithImpersonation(final FileSystem fs, public Object run() throws Exception { FileSystem fsAsUser = FileSystem.get(fs.getUri(), fs.getConf()); ShimLoader.getHadoopShims().checkFileAccess(fsAsUser, stat, action); + if(children != null) { + try { + FileStatus[] listStatus = fsAsUser.listStatus(stat.getPath()); + children.addAll(Arrays.asList(listStatus)); + } catch (Exception e) { + LOG.warn("Unable to list files under " + stat.getPath() + " : " + e); + throw e; + } + } return null; } }); @@ -426,20 +445,26 @@ public static boolean isActionPermittedForFileHierarchy(FileSystem fs, FileStatu dirActionNeeded.and(FsAction.EXECUTE); } + List subDirsToCheck = null; + if (isDir && recurse) { + subDirsToCheck = new ArrayList(); + } + try { - checkFileAccessWithImpersonation(fs, fileStatus, action, userName); + checkFileAccessWithImpersonation(fs, fileStatus, action, userName, subDirsToCheck); } catch (AccessControlException err) { // Action not permitted for user + LOG.warn("Action " + action + " denied on " + fileStatus.getPath() + " for user " + userName); return false; } - if ((!isDir) || (!recurse)) { + if (subDirsToCheck == null || subDirsToCheck.isEmpty()) { // no sub dirs to be checked return true; } + // check all children - FileStatus[] childStatuses = fs.listStatus(fileStatus.getPath()); - for (FileStatus childStatus : childStatuses) { + for (FileStatus childStatus : subDirsToCheck) { // check children recursively - recurse is true if we're here. if (!isActionPermittedForFileHierarchy(fs, childStatus, userName, action, true)) { return false; @@ -481,11 +506,30 @@ public static boolean isLocalFile(HiveConf conf, URI fileUri) { return false; } public static boolean isOwnerOfFileHierarchy(FileSystem fs, FileStatus fileStatus, String userName) - throws IOException { + throws IOException, InterruptedException { return isOwnerOfFileHierarchy(fs, fileStatus, userName, true); } - public static boolean isOwnerOfFileHierarchy(FileSystem fs, FileStatus fileStatus, + public static boolean isOwnerOfFileHierarchy(final FileSystem fs, + final FileStatus fileStatus, final String userName, final boolean recurse) + throws IOException, InterruptedException { + UserGroupInformation proxyUser = UserGroupInformation.createProxyUser(userName, + UserGroupInformation.getLoginUser()); + try { + boolean isOwner = proxyUser.doAs(new PrivilegedExceptionAction() { + @Override + public Boolean run() throws Exception { + FileSystem fsAsUser = FileSystem.get(fs.getUri(), fs.getConf()); + return checkIsOwnerOfFileHierarchy(fsAsUser, fileStatus, userName, recurse); + } + }); + return isOwner; + } finally { + FileSystem.closeAllForUGI(proxyUser); + } + } + + public static boolean checkIsOwnerOfFileHierarchy(FileSystem fs, FileStatus fileStatus, String userName, boolean recurse) throws IOException { if (!fileStatus.getOwner().equals(userName)) { @@ -500,7 +544,7 @@ public static boolean isOwnerOfFileHierarchy(FileSystem fs, FileStatus fileStatu FileStatus[] childStatuses = fs.listStatus(fileStatus.getPath()); for (FileStatus childStatus : childStatuses) { // check children recursively - recurse is true if we're here. - if (!isOwnerOfFileHierarchy(fs, childStatus, userName, true)) { + if (!checkIsOwnerOfFileHierarchy(fs, childStatus, userName, true)) { return false; } } diff --git a/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java b/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java index 94e059b5a585..462963ab0964 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java @@ -449,8 +449,8 @@ private static Set getPrivilegesFromFS( String userName, FileSystem fs, FileStatus fileStatus, boolean recurse) throws Exception { Set privs = new HashSet(); - LOG.debug("Checking fs privileges for {} {}", - fileStatus.toString(), (recurse? "recursively":"without recursion")); + LOG.info("Checking fs privileges of user {} for {} {} ", + userName, fileStatus.toString(), (recurse? "recursively":"without recursion")); if (FileUtils.isOwnerOfFileHierarchy(fs, fileStatus, userName, recurse)) { privs.add(SQLPrivTypeGrant.OWNER_PRIV); } From 687884bc0fee7c2eb98a2ed92ca988b3566b0cc5 Mon Sep 17 00:00:00 2001 From: Thejas M Nair Date: Sun, 23 Apr 2017 21:07:36 -0700 Subject: [PATCH 2/2] Fix case when impersonation is not needed for file access check --- .../apache/hadoop/hive/common/FileUtils.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java index f2f8f7fccad6..985fd8cfa3e9 100644 --- a/common/src/java/org/apache/hadoop/hive/common/FileUtils.java +++ b/common/src/java/org/apache/hadoop/hive/common/FileUtils.java @@ -390,6 +390,7 @@ public static void checkFileAccessWithImpersonation(final FileSystem fs, if (user == null || currentUser.equals(user)) { // No need to impersonate user, do the checks as the currently configured user. ShimLoader.getHadoopShims().checkFileAccess(fs, stat, action); + addChildren(fs, stat.getPath(), children); return; } @@ -402,15 +403,7 @@ public static void checkFileAccessWithImpersonation(final FileSystem fs, public Object run() throws Exception { FileSystem fsAsUser = FileSystem.get(fs.getUri(), fs.getConf()); ShimLoader.getHadoopShims().checkFileAccess(fsAsUser, stat, action); - if(children != null) { - try { - FileStatus[] listStatus = fsAsUser.listStatus(stat.getPath()); - children.addAll(Arrays.asList(listStatus)); - } catch (Exception e) { - LOG.warn("Unable to list files under " + stat.getPath() + " : " + e); - throw e; - } - } + addChildren(fsAsUser, stat.getPath(), children); return null; } }); @@ -419,6 +412,20 @@ public Object run() throws Exception { } } + private static void addChildren(FileSystem fsAsUser, Path path, List children) + throws IOException { + if (children != null) { + FileStatus[] listStatus; + try { + listStatus = fsAsUser.listStatus(path); + } catch (IOException e) { + LOG.warn("Unable to list files under " + path + " : " + e); + throw e; + } + children.addAll(Arrays.asList(listStatus)); + } + } + /** * Check if user userName has permissions to perform the given FsAction action * on all files under the file whose FileStatus fileStatus is provided