Repository: hadoop Updated Branches: refs/heads/branch-2 944fb1c3a -> 46c29b988
HDFS-7218. FSNamesystem ACL operations should write to audit log on failure. (clamb via yliu) Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/46c29b98 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/46c29b98 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/46c29b98 Branch: refs/heads/branch-2 Commit: 46c29b988d11149cd5f8cf8e50b0cc36cec19f40 Parents: 944fb1c Author: yliu <y...@apache.org> Authored: Wed Nov 5 15:53:08 2014 +0800 Committer: yliu <y...@apache.org> Committed: Wed Nov 5 15:53:08 2014 +0800 ---------------------------------------------------------------------- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 3 + .../hdfs/server/namenode/FSNamesystem.java | 26 +++++- .../hdfs/server/namenode/TestAuditLogger.java | 95 ++++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/46c29b98/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index c558530..cbac594 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -752,6 +752,9 @@ Release 2.6.0 - UNRELEASED HADOOP-11233. hadoop.security.kms.client.encrypted.key.cache.expiry property spelled wrong in core-default. (Stephen Chu via yliu) + HDFS-7218. FSNamesystem ACL operations should write to audit log on + failure. (clamb via yliu) + BREAKDOWN OF HDFS-6581 SUBTASKS AND RELATED JIRAS HDFS-6921. Add LazyPersist flag to FileStatus. (Arpit Agarwal) http://git-wip-us.apache.org/repos/asf/hadoop/blob/46c29b98/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java ---------------------------------------------------------------------- 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 54ad653..900bac1 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 @@ -7873,6 +7873,11 @@ public class FSNamesystem implements Namesystem, FSClusterStats, public FSDirectory getFSDirectory() { return dir; } + /** Set the FSDirectory. */ + @VisibleForTesting + public void setFSDirectory(FSDirectory dir) { + this.dir = dir; + } /** @return the cache manager. */ public CacheManager getCacheManager() { return cacheManager; @@ -8739,6 +8744,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List<AclEntry> newAcl = dir.modifyAclEntries(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "modifyAclEntries", srcArg); + throw e; } finally { writeUnlock(); } @@ -8763,6 +8771,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List<AclEntry> newAcl = dir.removeAclEntries(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "removeAclEntries", srcArg); + throw e; } finally { writeUnlock(); } @@ -8786,6 +8797,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List<AclEntry> newAcl = dir.removeDefaultAcl(src); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "removeDefaultAcl", srcArg); + throw e; } finally { writeUnlock(); } @@ -8809,6 +8823,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, dir.removeAcl(src); getEditLog().logSetAcl(src, AclFeature.EMPTY_ENTRY_LIST); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "removeAcl", srcArg); + throw e; } finally { writeUnlock(); } @@ -8832,6 +8849,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List<AclEntry> newAcl = dir.setAcl(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "setAcl", srcArg); + throw e; } finally { writeUnlock(); } @@ -8844,6 +8864,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats, FSPermissionChecker pc = getPermissionChecker(); checkOperation(OperationCategory.READ); byte[][] pathComponents = FSDirectory.getPathComponentsForReservedPath(src); + boolean success = false; readLock(); try { checkOperation(OperationCategory.READ); @@ -8851,9 +8872,12 @@ public class FSNamesystem implements Namesystem, FSClusterStats, if (isPermissionEnabled) { checkPermission(pc, src, false, null, null, null, null); } - return dir.getAclStatus(src); + final AclStatus ret = dir.getAclStatus(src); + success = true; + return ret; } finally { readUnlock(); + logAuditEvent(success, "getAclStatus", src); } } http://git-wip-us.apache.org/repos/asf/hadoop/blob/46c29b98/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java index 29fee68..e1e1c67 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java @@ -19,30 +19,39 @@ package org.apache.hadoop.hdfs.server.namenode; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_AUDIT_LOGGERS_KEY; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY; +import static org.apache.hadoop.test.GenericTestUtils.assertExceptionContains; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Matchers.anyListOf; +import static org.mockito.Matchers.anyString; import java.io.IOException; import java.net.HttpURLConnection; import java.net.InetAddress; import java.net.URI; import java.net.URISyntaxException; +import java.util.List; +import com.google.common.collect.Lists; import org.apache.hadoop.conf.Configuration; 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.FsPermission; import org.apache.hadoop.hdfs.HdfsConfiguration; import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.web.resources.GetOpParam; import org.apache.hadoop.ipc.RemoteException; import org.apache.hadoop.net.NetUtils; +import org.apache.hadoop.security.AccessControlException; import org.apache.hadoop.security.authorize.ProxyServers; import org.apache.hadoop.security.authorize.ProxyUsers; import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; /** * Tests for the {@link AuditLogger} custom audit logging interface. @@ -166,6 +175,87 @@ public class TestAuditLogger { } } + @Test + public void testAuditLogWithAclFailure() throws Exception { + final Configuration conf = new HdfsConfiguration(); + conf.setBoolean(DFS_NAMENODE_ACLS_ENABLED_KEY, true); + conf.set(DFS_NAMENODE_AUDIT_LOGGERS_KEY, + DummyAuditLogger.class.getName()); + final MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build(); + try { + cluster.waitClusterUp(); + final FSDirectory dir = cluster.getNamesystem().getFSDirectory(); + // Set up mock FSDirectory to test FSN audit logging during failure + final FSDirectory mockedDir = Mockito.spy(dir); + Mockito.doThrow(new AccessControlException("mock setAcl exception")). + when(mockedDir). + setAcl(anyString(), anyListOf(AclEntry.class)); + Mockito.doThrow(new AccessControlException("mock getAclStatus exception")). + when(mockedDir). + getAclStatus(anyString()); + Mockito.doThrow(new AccessControlException("mock removeAcl exception")). + when(mockedDir). + removeAcl(anyString()); + Mockito.doThrow(new AccessControlException("mock removeDefaultAcl exception")). + when(mockedDir). + removeDefaultAcl(anyString()); + Mockito.doThrow(new AccessControlException("mock removeAclEntries exception")). + when(mockedDir). + removeAclEntries(anyString(), anyListOf(AclEntry.class)); + Mockito.doThrow(new AccessControlException("mock modifyAclEntries exception")). + when(mockedDir). + modifyAclEntries(anyString(), anyListOf(AclEntry.class)); + // Replace the FSD with the mock FSD. + cluster.getNamesystem().setFSDirectory(mockedDir); + assertTrue(DummyAuditLogger.initialized); + DummyAuditLogger.resetLogCount(); + + final FileSystem fs = cluster.getFileSystem(); + final Path p = new Path("/"); + final List<AclEntry> acls = Lists.newArrayList(); + + try { + fs.getAclStatus(p); + } catch (AccessControlException e) { + assertExceptionContains("mock getAclStatus exception", e); + } + + try { + fs.setAcl(p, acls); + } catch (AccessControlException e) { + assertExceptionContains("mock setAcl exception", e); + } + + try { + fs.removeAcl(p); + } catch (AccessControlException e) { + assertExceptionContains("mock removeAcl exception", e); + } + + try { + fs.removeDefaultAcl(p); + } catch (AccessControlException e) { + assertExceptionContains("mock removeDefaultAcl exception", e); + } + + try { + fs.removeAclEntries(p, acls); + } catch (AccessControlException e) { + assertExceptionContains("mock removeAclEntries exception", e); + } + + try { + fs.modifyAclEntries(p, acls); + } catch (AccessControlException e) { + assertExceptionContains("mock modifyAclEntries exception", e); + } + assertEquals(6, DummyAuditLogger.logCount); + assertEquals(6, DummyAuditLogger.unsuccessfulCount); + } finally { + cluster.shutdown(); + } + } + /** * Tests that a broken audit logger causes requests to fail. */ @@ -194,6 +284,7 @@ public class TestAuditLogger { static boolean initialized; static int logCount; + static int unsuccessfulCount; static short foundPermission; static String remoteAddr; @@ -203,6 +294,7 @@ public class TestAuditLogger { public static void resetLogCount() { logCount = 0; + unsuccessfulCount = 0; } public void logAuditEvent(boolean succeeded, String userName, @@ -210,6 +302,9 @@ public class TestAuditLogger { FileStatus stat) { remoteAddr = addr.getHostAddress(); logCount++; + if (!succeeded) { + unsuccessfulCount++; + } if (stat != null) { foundPermission = stat.getPermission().toShort(); }
