HBASE-11434 [AccessController] Disallow inbound cells with reserved tags

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<InternalScanner,String> scannerOwners =
       new MapMaker().weakKeys().makeMap();
 
+  // Provider for mapping principal names to Users
   private UserProvider userProvider;
 
+  // The list of users with superuser authority
+  private List<String> 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<byte[], List<Cell>> familyMap) {
+  private static void addCellPermissions(final byte[] perms, Map<byte[], List<Cell>> familyMap) {
     // Iterate over the entries in the familyMap, replacing the cells therein
     // with new cells including the ACL data
     for (Map.Entry<byte[], List<Cell>> e: familyMap.entrySet()) {
@@ -772,6 +778,33 @@ public class AccessController extends BaseRegionObserver
       e.setValue(newCells);
     }
   }
+
+  // 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<Tag> 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 {
@@ -808,6 +841,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) {
@@ -1451,9 +1489,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<byte[],? extends Collection<Cell>> families = put.getFamilyCellMap();
-    User user = getActiveUser();
     AuthResult authResult = permissionGranted(OpType.PUT, user, env, families, Action.WRITE);
     logResult(authResult);
     if (!authResult.isAllowed()) {
@@ -1463,6 +1502,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) {
@@ -1518,7 +1558,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)) {
@@ -1554,9 +1600,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<byte[],? extends Collection<byte[]>> families = makeFamilyMap(family, qualifier);
-    User user = getActiveUser();
     AuthResult authResult = permissionGranted(OpType.CHECK_AND_PUT, user, env, families,
       Action.READ, Action.WRITE);
     logResult(authResult);
@@ -1690,9 +1737,10 @@ public class AccessController extends BaseRegionObserver
   public Result preAppend(ObserverContext<RegionCoprocessorEnvironment> 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<byte[],? extends Collection<Cell>> families = append.getFamilyCellMap();
-    User user = getActiveUser();
     AuthResult authResult = permissionGranted(OpType.APPEND, user, env, families, Action.WRITE);
     logResult(authResult);
     if (!authResult.isAllowed()) {
@@ -1743,9 +1791,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<byte[],? extends Collection<Cell>> families = increment.getFamilyCellMap();
-    User user = getActiveUser();
     AuthResult authResult = permissionGranted(OpType.INCREMENT, user, env, families,
       Action.WRITE);
     logResult(authResult);
@@ -2219,11 +2268,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<String> 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") +

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;
@@ -2126,4 +2125,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);
+  }
+
 }