From 73e601259fed0646f115b09112995b51ffef3468 Mon Sep 17 00:00:00 2001 From: yliu Date: Wed, 5 Nov 2014 15:49:37 +0800 Subject: [PATCH] HDFS-7218. FSNamesystem ACL operations should write to audit log on failure. (clamb via yliu) --- 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(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 098d4bacd7e..707929e257f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -1010,6 +1010,9 @@ Release 2.6.0 - UNRELEASED fails on Windows, because we cannot deny access to the file owner. (Chris Nauroth via wheat9) + HDFS-7218. FSNamesystem ACL operations should write to audit log on + failure. (clamb via yliu) + BREAKDOWN OF HDFS-6134 AND HADOOP-10150 SUBTASKS AND RELATED JIRAS HDFS-6387. HDFS CLI admin tool for creating & deleting an 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 2bc4ba0f42a..76c14234c7d 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 @@ -7862,6 +7862,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; @@ -8728,6 +8733,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List newAcl = dir.modifyAclEntries(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "modifyAclEntries", srcArg); + throw e; } finally { writeUnlock(); } @@ -8752,6 +8760,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List newAcl = dir.removeAclEntries(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "removeAclEntries", srcArg); + throw e; } finally { writeUnlock(); } @@ -8775,6 +8786,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List newAcl = dir.removeDefaultAcl(src); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "removeDefaultAcl", srcArg); + throw e; } finally { writeUnlock(); } @@ -8798,6 +8812,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(); } @@ -8821,6 +8838,9 @@ public class FSNamesystem implements Namesystem, FSClusterStats, List newAcl = dir.setAcl(src, aclSpec); getEditLog().logSetAcl(src, newAcl); resultingStat = getAuditFileInfo(src, false); + } catch (AccessControlException e) { + logAuditEvent(false, "setAcl", srcArg); + throw e; } finally { writeUnlock(); } @@ -8833,6 +8853,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); @@ -8840,9 +8861,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); } } 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 29fee68a06e..e1e1c6797b4 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 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(); }