From 62e6d8ff31257985dea473a731a304dd14dda029 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Mon, 30 Jun 2014 09:54:33 -0700 Subject: [PATCH] HBASE-11434 [AccessController] Disallow inbound cells with reserved tags --- .../hbase/security/access/AccessController.java | 65 ++++++++++++++++++---- .../security/access/TestAccessController.java | 29 +++++++++- 2 files changed, 81 insertions(+), 13 deletions(-) diff --git hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index 588a216..21f8892 100644 --- hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -31,6 +31,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellScanner; import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.CompoundConfiguration; import org.apache.hadoop.hbase.CoprocessorEnvironment; @@ -152,6 +153,7 @@ public class AccessController extends BaseRegionObserver private static final Log AUDITLOG = LogFactory.getLog("SecurityLogger."+AccessController.class.getName()); private static final String CHECK_COVERING_PERM = "check_covering_perm"; + private static final String TAG_CHECK_PASSED = "tag_check_passed"; private static final byte[] TRUE = Bytes.toBytes(true); TableAuthManager authManager = null; @@ -167,8 +169,12 @@ public class AccessController extends BaseRegionObserver private Map scannerOwners = new MapMaker().weakKeys().makeMap(); + // Provider for mapping principal names to Users private UserProvider userProvider; + // The list of users with superuser authority + private List superusers; + // if we are able to support cell ACLs boolean cellFeaturesEnabled; @@ -745,7 +751,7 @@ public class AccessController extends BaseRegionObserver return cellGrants > 0; } - private void addCellPermissions(final byte[] perms, Map> familyMap) { + private static void addCellPermissions(final byte[] perms, Map> familyMap) { // Iterate over the entries in the familyMap, replacing the cells therein // with new cells including the ACL data for (Map.Entry> e: familyMap.entrySet()) { @@ -774,6 +780,32 @@ public class AccessController extends BaseRegionObserver } } + // Checks whether incoming cells contain any tag with type as ACL_TAG_TYPE. This tag + // type is reserved and should not be explicitly set by user. + private void checkForReservedTagPresence(User user, Mutation m) throws IOException { + // Superusers are allowed to store cells unconditionally. + if (superusers.contains(user.getShortName())) { + return; + } + // We already checked (prePut vs preBatchMutation) + if (m.getAttribute(TAG_CHECK_PASSED) != null) { + return; + } + for (CellScanner cellScanner = m.cellScanner(); cellScanner.advance();) { + Cell cell = cellScanner.current(); + if (cell.getTagsLength() > 0) { + Iterator tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), + cell.getTagsLength()); + while (tagsItr.hasNext()) { + if (tagsItr.next().getType() == AccessControlLists.ACL_TAG_TYPE) { + throw new AccessDeniedException("Mutation contains cell with reserved type tag"); + } + } + } + } + m.setAttribute(TAG_CHECK_PASSED, TRUE); + } + /* ---- MasterObserver implementation ---- */ public void start(CoprocessorEnvironment env) throws IOException { @@ -810,6 +842,11 @@ public class AccessController extends BaseRegionObserver // set the user-provider. this.userProvider = UserProvider.instantiate(env.getConfiguration()); + // set up the list of users with superuser privilege + User user = userProvider.getCurrent(); + superusers = Lists.asList(user.getShortName(), + conf.getStrings(AccessControlLists.SUPERUSER_CONF_KEY, new String[0])); + // If zk is null or IOException while obtaining auth manager, // throw RuntimeException so that the coprocessor is unloaded. if (zk != null) { @@ -1426,9 +1463,10 @@ public class AccessController extends BaseRegionObserver // HBase value. A new ACL in a new Put applies to that Put. It doesn't // change the ACL of any previous Put. This allows simple evolution of // security policy over time without requiring expensive updates. + User user = getActiveUser(); + checkForReservedTagPresence(user, put); RegionCoprocessorEnvironment env = c.getEnvironment(); Map> families = put.getFamilyCellMap(); - User user = getActiveUser(); AuthResult authResult = permissionGranted(OpType.PUT, user, env, families, Action.WRITE); logResult(authResult); if (!authResult.isAllowed()) { @@ -1438,6 +1476,7 @@ public class AccessController extends BaseRegionObserver throw new AccessDeniedException("Insufficient permissions " + authResult.toContextString()); } } + // Add cell ACLs from the operation to the cells themselves byte[] bytes = put.getAttribute(AccessControlConstants.OP_ATTRIBUTE_ACL); if (bytes != null) { if (cellFeaturesEnabled) { @@ -1493,7 +1532,13 @@ public class AccessController extends BaseRegionObserver if (m.getAttribute(CHECK_COVERING_PERM) != null) { // We have a failure with table, cf and q perm checks and now giving a chance for cell // perm check - OpType opType = (m instanceof Put) ? OpType.PUT : OpType.DELETE; + OpType opType; + if (m instanceof Put) { + checkForReservedTagPresence(getActiveUser(), m); + opType = OpType.PUT; + } else { + opType = OpType.DELETE; + } AuthResult authResult = null; if (checkCoveringPermission(opType, c.getEnvironment(), m.getRow(), m.getFamilyCellMap(), m.getTimeStamp(), Action.WRITE)) { @@ -1529,9 +1574,10 @@ public class AccessController extends BaseRegionObserver final ByteArrayComparable comparator, final Put put, final boolean result) throws IOException { // Require READ and WRITE permissions on the table, CF, and KV to update + User user = getActiveUser(); + checkForReservedTagPresence(user, put); RegionCoprocessorEnvironment env = c.getEnvironment(); Map> families = makeFamilyMap(family, qualifier); - User user = getActiveUser(); AuthResult authResult = permissionGranted(OpType.CHECK_AND_PUT, user, env, families, Action.READ, Action.WRITE); logResult(authResult); @@ -1665,9 +1711,10 @@ public class AccessController extends BaseRegionObserver public Result preAppend(ObserverContext c, Append append) throws IOException { // Require WRITE permission to the table, CF, and the KV to be appended + User user = getActiveUser(); + checkForReservedTagPresence(user, append); RegionCoprocessorEnvironment env = c.getEnvironment(); Map> families = append.getFamilyCellMap(); - User user = getActiveUser(); AuthResult authResult = permissionGranted(OpType.APPEND, user, env, families, Action.WRITE); logResult(authResult); if (!authResult.isAllowed()) { @@ -1718,9 +1765,10 @@ public class AccessController extends BaseRegionObserver throws IOException { // Require WRITE permission to the table, CF, and the KV to be replaced by // the incremented value + User user = getActiveUser(); + checkForReservedTagPresence(user, increment); RegionCoprocessorEnvironment env = c.getEnvironment(); Map> families = increment.getFamilyCellMap(); - User user = getActiveUser(); AuthResult authResult = permissionGranted(OpType.INCREMENT, user, env, families, Action.WRITE); logResult(authResult); @@ -2195,11 +2243,6 @@ public class AccessController extends BaseRegionObserver throw new IOException("Unable to obtain the current user, " + "authorization checks for internal operations will not work correctly!"); } - - String currentUser = user.getShortName(); - List superusers = Lists.asList(currentUser, conf.getStrings( - AccessControlLists.SUPERUSER_CONF_KEY, new String[0])); - User activeUser = getActiveUser(); if (!(superusers.contains(activeUser.getShortName()))) { throw new AccessDeniedException("User '" + (user != null ? user.getShortName() : "null") + diff --git hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java index 2d8374f..df1544f 100644 --- hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java +++ hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java @@ -48,6 +48,7 @@ import org.apache.hadoop.hbase.MiniHBaseCluster; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotFoundException; +import org.apache.hadoop.hbase.Tag; import org.apache.hadoop.hbase.client.Append; import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Get; @@ -94,10 +95,8 @@ import org.apache.hadoop.hbase.security.access.Permission.Action; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.JVMClusterUtil; import org.apache.hadoop.hbase.util.TestTableName; - import org.apache.log4j.Level; import org.apache.log4j.Logger; - import org.junit.After; import org.junit.AfterClass; import org.junit.Before; @@ -2109,4 +2108,30 @@ public class TestAccessController extends SecureTestUtil { verifyAllowed(execEndpointAction, userA, userB); } + @Test + public void testReservedCellTags() throws Exception { + AccessTestAction putWithReservedTag = new AccessTestAction() { + @Override + public Object run() throws Exception { + HTable t = new HTable(conf, TEST_TABLE.getTableName()); + try { + KeyValue kv = new KeyValue(TEST_ROW, TEST_FAMILY, TEST_QUALIFIER, + HConstants.LATEST_TIMESTAMP, HConstants.EMPTY_BYTE_ARRAY, + new Tag[] { new Tag(AccessControlLists.ACL_TAG_TYPE, + ProtobufUtil.toUsersAndPermissions(USER_OWNER.getShortName(), + new Permission(Permission.Action.READ)).toByteArray()) }); + t.put(new Put(TEST_ROW).add(kv)); + } finally { + t.close(); + } + return null; + } + }; + + // Current user is superuser + verifyAllowed(putWithReservedTag, User.getCurrent()); + // No other user should be allowed + verifyDenied(putWithReservedTag, USER_OWNER, USER_ADMIN, USER_CREATE, USER_RW, USER_RO); + } + } -- 1.9.1