commit 49f4cbe38fa21f10f39d6138ffe86c5505693fa1 Author: Chris Drome Date: Fri Jun 10 00:48:08 2016 +0000 HIVE-13989: Extended ACLs are not handled according to specification diff --git hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java index 367f4ea..4fb09e7 100644 --- hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java +++ hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java @@ -32,6 +32,10 @@ import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.AclEntry; +import org.apache.hadoop.fs.permission.AclEntryScope; +import org.apache.hadoop.fs.permission.AclEntryType; +import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hive.common.FileUtils; import org.apache.hadoop.hive.conf.HiveConf; @@ -331,7 +335,7 @@ private Partition constructPartition( String partLocnRoot, String dynPartPath, Map partKVs, HCatSchema outputSchema, Map params, Table table, FileSystem fs, - String grpName, FsPermission perms) throws IOException { + String grpName, FsPermission perms, List acls) throws IOException { Partition partition = new Partition(); partition.setDbName(table.getDbName()); @@ -368,7 +372,7 @@ private Partition constructPartition( for (FieldSchema partKey : table.getPartitionKeys()) { if (i++ != 0) { fs.mkdirs(partPath); // Attempt to make the path in case it does not exist before we check - applyGroupAndPerms(fs, partPath, perms, grpName, false); + applyGroupAndPerms(fs, partPath, perms, acls, grpName, false); } partPath = constructPartialPartPath(partPath, partKey.getName().toLowerCase(), partKVs); } @@ -378,7 +382,7 @@ private Partition constructPartition( // Need not bother in case of HDFS as permission is taken care of by setting UMask fs.mkdirs(partPath); // Attempt to make the path in case it does not exist before we check if (!ShimLoader.getHadoopShims().getHCatShim().isFileInHDFS(fs, partPath)) { - applyGroupAndPerms(fs, partPath, perms, grpName, true); + applyGroupAndPerms(fs, partPath, perms, acls, grpName, true); } // Set the location in the StorageDescriptor @@ -398,7 +402,7 @@ private Partition constructPartition( } private void applyGroupAndPerms(FileSystem fs, Path dir, FsPermission permission, - String group, boolean recursive) + List acls, String group, boolean recursive) throws IOException { if(LOG.isDebugEnabled()) { LOG.debug("applyGroupAndPerms : " + dir + @@ -406,10 +410,13 @@ private void applyGroupAndPerms(FileSystem fs, Path dir, FsPermission permission " group: " + group + " recursive: " + recursive); } fs.setPermission(dir, permission); + if ((acls != null) && (acls.size() > 0)) { + fs.setAcl(dir, acls); + } if (recursive) { for (FileStatus fileStatus : fs.listStatus(dir)) { if (fileStatus.isDir()) { - applyGroupAndPerms(fs, fileStatus.getPath(), permission, group, true); + applyGroupAndPerms(fs, fileStatus.getPath(), permission, acls, group, true); } else { fs.setPermission(fileStatus.getPath(), permission); } @@ -786,6 +793,18 @@ private void registerPartitions(JobContext context) throws IOException{ FileStatus tblStat = fs.getFileStatus(tblPath); String grpName = tblStat.getGroup(); FsPermission perms = tblStat.getPermission(); + ArrayList acls = null; + + if (conf.getBoolean("dfs.namenode.acls.enabled", false)) { + try { + AclStatus stat = fs.getAclStatus(tblPath); + if (hasExtendedAcls(stat)) { + acls = getDefaultAclEntries(stat, perms); + } + } catch (UnsupportedOperationException e) { + LOG.debug("Skipping ACLs", e); + } + } List partitionsToAdd = new ArrayList(); if (!dynamicPartitioningUsed){ @@ -795,7 +814,7 @@ private void registerPartitions(JobContext context) throws IOException{ tblPath.toString(), null, jobInfo.getPartitionValues() ,jobInfo.getOutputSchema(), getStorerParameterMap(storer) ,table, fs - ,grpName,perms)); + ,grpName,perms, acls)); }else{ for (Entry> entry : partitionsDiscoveredByPath.entrySet()){ partitionsToAdd.add( @@ -805,7 +824,7 @@ private void registerPartitions(JobContext context) throws IOException{ ,entry.getKey(), entry.getValue() ,jobInfo.getOutputSchema(), getStorerParameterMap(storer) ,table, fs - ,grpName,perms)); + ,grpName,perms, acls)); } } @@ -941,7 +960,7 @@ private void registerPartitions(JobContext context) throws IOException{ // Set permissions appropriately for each of the partitions we just created // so as to have their permissions mimic the table permissions for (Partition p : partitionsAdded){ - applyGroupAndPerms(fs,new Path(p.getSd().getLocation()),tblStat.getPermission(),tblStat.getGroup(),true); + applyGroupAndPerms(fs,new Path(p.getSd().getLocation()),tblStat.getPermission(),acls,tblStat.getGroup(),true); } } @@ -1015,5 +1034,80 @@ private void cancelDelegationTokens(JobContext context) throws IOException{ } } + private ArrayList getDefaultAclEntries(AclStatus stat, FsPermission perms) { + ArrayList defaults = new ArrayList(); + + boolean[] hasDefaults = { false, false, false, false }; + + for (AclEntry e : stat.getEntries()) { + if (e.getScope() == AclEntryScope.DEFAULT) { + AclEntry acl = new AclEntry.Builder().setName(e.getName()).setScope(AclEntryScope.ACCESS) + .setType(e.getType()).setPermission(e.getPermission()).build(); + + defaults.add(acl); + defaults.add(e); + + if (e.getName() == null) { + if (e.getType() == AclEntryType.USER) { + hasDefaults[0] = true; + } + if (e.getType() == AclEntryType.GROUP) { + hasDefaults[1] = true; + } + if (e.getType() == AclEntryType.OTHER) { + hasDefaults[2] = true; + } + if (e.getType() == AclEntryType.MASK) { + hasDefaults[3] = true; + } + } + } + } + + if (! hasDefaults[0]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.USER).setPermission(perms.getUserAction()).build(); + defaults.add(acl); + hasDefaults[0] = true; + } + + if (! hasDefaults[1]) { + for (AclEntry e : stat.getEntries()) { + if ((e.getScope() == AclEntryScope.ACCESS) && (e.getType() == AclEntryType.GROUP) && (e.getName() == null)) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.GROUP).setPermission(e.getPermission()).build(); + defaults.add(acl); + + hasDefaults[1] = true; + } + } + } + + if (! hasDefaults[2]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.OTHER).setPermission(perms.getOtherAction()).build(); + defaults.add(acl); + + hasDefaults[2] = true; + } + + if (! hasDefaults[3]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.MASK).setPermission(perms.getGroupAction()).build(); + defaults.add(acl); + + hasDefaults[3] = true; + } + + return defaults; + } + + private boolean hasExtendedAcls(AclStatus status) { + if (status != null) { + return status.getEntries().size() > 0; + } + + return false; + } } diff --git itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestExtendedAcls.java itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestExtendedAcls.java index d846a63..a757e64 100644 --- itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestExtendedAcls.java +++ itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestExtendedAcls.java @@ -18,12 +18,14 @@ package org.apache.hadoop.hive.ql.security; import static org.apache.hadoop.fs.permission.AclEntryScope.ACCESS; +import static org.apache.hadoop.fs.permission.AclEntryScope.DEFAULT; import static org.apache.hadoop.fs.permission.AclEntryType.GROUP; import static org.apache.hadoop.fs.permission.AclEntryType.OTHER; import static org.apache.hadoop.fs.permission.AclEntryType.USER; import java.util.List; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.AclEntry; import org.apache.hadoop.fs.permission.AclEntryScope; @@ -54,7 +56,11 @@ public static void setup() throws Exception { aclEntry(ACCESS, USER, "bar", FsAction.READ_WRITE), aclEntry(ACCESS, USER, "foo", FsAction.READ_EXECUTE), aclEntry(ACCESS, GROUP, "bar", FsAction.READ_WRITE), - aclEntry(ACCESS, GROUP, "foo", FsAction.READ_EXECUTE)); + aclEntry(ACCESS, GROUP, "foo", FsAction.READ_EXECUTE), + aclEntry(DEFAULT, USER, "bar", FsAction.READ_WRITE), + aclEntry(DEFAULT, USER, "foo", FsAction.READ_EXECUTE), + aclEntry(DEFAULT, GROUP, "bar", FsAction.READ_WRITE), + aclEntry(DEFAULT, GROUP, "foo", FsAction.READ_EXECUTE)); private final ImmutableList aclSpec2 = ImmutableList.of( aclEntry(ACCESS, USER, FsAction.ALL), @@ -63,7 +69,11 @@ public static void setup() throws Exception { aclEntry(ACCESS, USER, "bar2", FsAction.READ_WRITE), aclEntry(ACCESS, USER, "foo2", FsAction.READ_EXECUTE), aclEntry(ACCESS, GROUP, "bar2", FsAction.READ), - aclEntry(ACCESS, GROUP, "foo2", FsAction.READ_EXECUTE)); + aclEntry(ACCESS, GROUP, "foo2", FsAction.READ_EXECUTE), + aclEntry(DEFAULT, USER, "bar2", FsAction.READ_WRITE), + aclEntry(DEFAULT, USER, "foo2", FsAction.READ_EXECUTE), + aclEntry(DEFAULT, GROUP, "bar2", FsAction.READ), + aclEntry(DEFAULT, GROUP, "foo2", FsAction.READ_EXECUTE)); @Override public void setPermission(String locn, int permIndex) throws Exception { @@ -81,20 +91,21 @@ public void setPermission(String locn, int permIndex) throws Exception { @Override public void verifyPermission(String locn, int permIndex) throws Exception { + FileStatus fstat = fs.getFileStatus(new Path(locn)); + FsPermission perm = fstat.getPermission(); + switch (permIndex) { case 0: - FsPermission perm = fs.getFileStatus(new Path(locn)).getPermission(); Assert.assertEquals("Location: " + locn, "rwxrwxrwx", String.valueOf(perm)); List actual = getAcl(locn); - verifyAcls(aclSpec1, actual); + verifyAcls(aclSpec1, actual, fstat.isFile()); break; case 1: - perm = fs.getFileStatus(new Path(locn)).getPermission(); Assert.assertEquals("Location: " + locn, "rwxrwxr-x", String.valueOf(perm)); List acls = getAcl(locn); - verifyAcls(aclSpec2, acls); + verifyAcls(aclSpec2, acls, fstat.isFile()); break; default: throw new RuntimeException("Only 2 permissions by this test: " + permIndex); @@ -137,9 +148,14 @@ private AclEntry aclEntry(AclEntryScope scope, AclEntryType type, .setPermission(permission).build(); } - private void verifyAcls(List expectedList, List actualList) { + private void verifyAcls(List expectedList, List actualList, boolean isFile) { for (AclEntry expected : expectedList) { if (expected.getName() != null) { + if (isFile && expected.getScope() == DEFAULT) { + // Files will not inherit default extended ACL rules from its parent, so ignore them. + continue; + } + //the non-named acl's are coming as regular permission, and not as aclEntries. boolean found = false; for (AclEntry actual : actualList) { diff --git shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java index 2e09882..cecc6b5 100644 --- shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java +++ shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java @@ -755,20 +755,13 @@ public void setFullFileStatus(Configuration conf, HdfsFileStatus sourceStatus, if (isExtendedAclEnabled(conf)) { //Attempt extended Acl operations only if its enabled, 8791but don't fail the operation regardless. try { - AclStatus aclStatus = ((Hadoop23FileStatus) sourceStatus).getAclStatus(); - if (aclStatus != null) { - List aclEntries = aclStatus.getEntries(); - removeBaseAclEntries(aclEntries); - - //the ACL api's also expect the tradition user/group/other permission in the form of ACL - FsPermission sourcePerm = sourceStatus.getFileStatus().getPermission(); - aclEntries.add(newAclEntry(AclEntryScope.ACCESS, AclEntryType.USER, sourcePerm.getUserAction())); - aclEntries.add(newAclEntry(AclEntryScope.ACCESS, AclEntryType.GROUP, sourcePerm.getGroupAction())); - aclEntries.add(newAclEntry(AclEntryScope.ACCESS, AclEntryType.OTHER, sourcePerm.getOtherAction())); - - //construct the -setfacl command - String aclEntry = Joiner.on(",").join(aclStatus.getEntries()); + if (hasExtendedAcls((Hadoop23FileStatus) sourceStatus)) { + List aclEntries = getDefaultAclEntries((Hadoop23FileStatus) sourceStatus); + String aclEntry = Joiner.on(",").join(aclEntries); run(fsShell, new String[]{"-setfacl", "-R", "--set", aclEntry, target.toString()}); + } else { + String permission = Integer.toString(sourceStatus.getFileStatus().getPermission().toShort(), 8); + run(fsShell, new String[]{"-chmod", "-R", permission, target.toString()}); } } catch (Exception e) { LOG.info("Skipping ACL inheritance: File system for path " + target + " " + @@ -833,19 +826,95 @@ private AclEntry newAclEntry(AclEntryScope scope, AclEntryType type, } /** - * Removes basic permission acls (unamed acls) from the list of acl entries - * @param entries acl entries to remove from. + * Generates a list of AclEntrys as determined from src. + * First iterate through all of the AclEntrys and extract the DEFAULT rules, from which + * we will create an ACCESS rule as well. If DEFAULT rules were not found for user, group, other, or + * mask, then inherit them from the basic permissions of src. Because the group permission + * is now the mask, we need to pull the group permission from list of extended ACLs. + * + * @param src the FileStatus from which to inherit permissions. + * @return a list of AclEntrys to inherit from src. */ - private void removeBaseAclEntries(List entries) { - Iterables.removeIf(entries, new Predicate() { - @Override - public boolean apply(AclEntry input) { - if (input.getName() == null) { - return true; + private List getDefaultAclEntries(Hadoop23FileStatus src) { + ArrayList defaults = new ArrayList(); + + boolean[] hasDefaults = { false, false, false, false }; + + for (AclEntry e : src.getAclStatus().getEntries()) { + if (e.getScope() == AclEntryScope.DEFAULT) { + AclEntry acl = new AclEntry.Builder().setName(e.getName()).setScope(AclEntryScope.ACCESS) + .setType(e.getType()).setPermission(e.getPermission()).build(); + + defaults.add(acl); + defaults.add(e); + + if (e.getName() == null) { + if (e.getType() == AclEntryType.USER) { + hasDefaults[0] = true; + } + if (e.getType() == AclEntryType.GROUP) { + hasDefaults[1] = true; + } + if (e.getType() == AclEntryType.OTHER) { + hasDefaults[2] = true; } - return false; + if (e.getType() == AclEntryType.MASK) { + hasDefaults[3] = true; + } + } + } + } + + if (! hasDefaults[0]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.USER).setPermission(src.getFileStatus().getPermission().getUserAction()).build(); + defaults.add(acl); + + hasDefaults[0] = true; + } + + if (! hasDefaults[1]) { + for (AclEntry e : src.getAclStatus().getEntries()) { + if ((e.getScope() == AclEntryScope.ACCESS) && (e.getType() == AclEntryType.GROUP) && (e.getName() == null)) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.GROUP).setPermission(e.getPermission()).build(); + defaults.add(acl); + + hasDefaults[1] = true; + } } - }); + } + + if (! hasDefaults[2]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.OTHER).setPermission(src.getFileStatus().getPermission().getOtherAction()).build(); + defaults.add(acl); + + hasDefaults[2] = true; + } + + if (! hasDefaults[3]) { + AclEntry acl = new AclEntry.Builder().setScope(AclEntryScope.ACCESS) + .setType(AclEntryType.MASK).setPermission(src.getFileStatus().getPermission().getGroupAction()).build(); + defaults.add(acl); + + hasDefaults[3] = true; + } + + return defaults; + } + + /** + * Returns true if extended ACL rules exist, otherwise false. + * @param status the FileStatus containing access permissions. + * @return true if extended ACL rules exist, otherwise false. + */ + private boolean hasExtendedAcls(Hadoop23FileStatus status) { + if (status.getAclStatus() != null) { + return status.getAclStatus().getEntries().size() > 0; + } + + return false; } class ProxyFileSystem23 extends ProxyFileSystem {