Reduce the effective scope of CREATE and ADMIN permissions

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -291,21 +291,6 @@ public class AccessController extends BaseMasterAndRegionObserver
         permRequest, tableName, families);
     }
 
-    // Users with CREATE/ADMIN rights need to modify hbase:meta and _acl_ table
-    // e.g. When a new table is created a new entry in hbase:meta is added,
-    // so the user need to be allowed to write on it.
-    // e.g. When a table is removed an entry is removed from hbase:meta and _acl_
-    // and the user need to be allowed to write on both tables.
-    if (permRequest == Action.WRITE &&
-       (hri.isMetaRegion() ||
-        Bytes.equals(tableName.getName(), AccessControlLists.ACL_GLOBAL_NAME)) &&
-       (authManager.authorize(user, Action.CREATE) ||
-        authManager.authorize(user, Action.ADMIN)))
-    {
-       return AuthResult.allow(request, "Table permission granted", user,
-        permRequest, tableName, families);
-    }
-
     // 2. check for the table-level, if successful we can short-circuit
     if (authManager.authorize(user, tableName, (byte[])null, permRequest)) {
       return AuthResult.allow(request, "Table permission granted", user,
@@ -931,31 +916,51 @@ public class AccessController extends BaseMasterAndRegionObserver
 
   @Override
   public void postDeleteTable(ObserverContext<MasterCoprocessorEnvironment> c,
-      TableName tableName) throws IOException {
+      final TableName tableName) throws IOException {
-    AccessControlLists.removeTablePermissions(c.getEnvironment().getConfiguration(), tableName);
+    final Configuration conf = c.getEnvironment().getConfiguration();
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
+      @Override
+      public Void run() throws Exception {
+        AccessControlLists.removeTablePermissions(conf, tableName);
+        return null;
+      }
+    });
   }
 
   @Override
-  public void preTruncateTable(ObserverContext<MasterCoprocessorEnvironment> c, TableName tableName)
+  public void preTruncateTable(ObserverContext<MasterCoprocessorEnvironment> c,
-      throws IOException {
+      final TableName tableName) throws IOException {
     requirePermission("truncateTable", tableName, null, null, Action.ADMIN);
-    List<UserPermission> acls = AccessControlLists.getUserTablePermissions(c.getEnvironment()
+    final Configuration conf = c.getEnvironment().getConfiguration();
-        .getConfiguration(), tableName);
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
-    if (acls != null) {
+      @Override
-      tableAcls.put(tableName, acls);
+      public Void run() throws Exception {
-    }
+        List<UserPermission> acls = AccessControlLists.getUserTablePermissions(conf, tableName);
+        if (acls != null) {
+          tableAcls.put(tableName, acls);
+        }
+        return null;
+      }
+    });
   }
 
   @Override
   public void postTruncateTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
-      TableName tableName) throws IOException {
+      final TableName tableName) throws IOException {
-    List<UserPermission> perms = tableAcls.get(tableName);
+    final Configuration conf = ctx.getEnvironment().getConfiguration();
-    if (perms != null) {
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
-      for (UserPermission perm : perms) {
+      @Override
-        AccessControlLists.addUserPermission(ctx.getEnvironment().getConfiguration(), perm);
+      public Void run() throws Exception {
+        List<UserPermission> perms = tableAcls.get(tableName);
+        if (perms != null) {
+          for (UserPermission perm : perms) {
+            AccessControlLists.addUserPermission(conf, perm);
+          }
+        }
+        tableAcls.remove(tableName);
+        return null;
       }
-    }
+    });
-    tableAcls.remove(tableName);
   }
 
   @Override
@@ -966,13 +971,20 @@ public class AccessController extends BaseMasterAndRegionObserver
 
   @Override
   public void postModifyTable(ObserverContext<MasterCoprocessorEnvironment> c,
-      TableName tableName, HTableDescriptor htd) throws IOException {
+      TableName tableName, final HTableDescriptor htd) throws IOException {
-    String owner = htd.getOwnerString();
+    final Configuration conf = c.getEnvironment().getConfiguration();
     // default the table owner to current user, if not specified.
-    if (owner == null) owner = getActiveUser().getShortName();
+    final String owner = (htd.getOwnerString() != null) ? htd.getOwnerString() : 
-    UserPermission userperm = new UserPermission(Bytes.toBytes(owner), htd.getTableName(), null,
+      getActiveUser().getShortName();
-        Action.values());
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
-    AccessControlLists.addUserPermission(c.getEnvironment().getConfiguration(), userperm);
+      @Override
+      public Void run() throws Exception {
+        UserPermission userperm = new UserPermission(Bytes.toBytes(owner),
+          htd.getTableName(), null, Action.values());
+        AccessControlLists.addUserPermission(conf, userperm);
+        return null;
+      }
+    });
   }
 
   @Override
@@ -995,8 +1007,15 @@ public class AccessController extends BaseMasterAndRegionObserver
 
   @Override
   public void postDeleteColumn(ObserverContext<MasterCoprocessorEnvironment> c,
-      TableName tableName, byte[] col) throws IOException {
+      final TableName tableName, final byte[] col) throws IOException {
-    AccessControlLists.removeTablePermissions(c.getEnvironment().getConfiguration(), tableName, col);
+    final Configuration conf = c.getEnvironment().getConfiguration();
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
+      @Override
+      public Void run() throws Exception {
+        AccessControlLists.removeTablePermissions(conf, tableName, col);
+        return null;
+      }
+    });
   }
 
   @Override
@@ -1117,9 +1136,15 @@ public class AccessController extends BaseMasterAndRegionObserver
 
   @Override
   public void postDeleteNamespace(ObserverContext<MasterCoprocessorEnvironment> ctx,
-                                  String namespace) throws IOException {
+      final String namespace) throws IOException {
-    AccessControlLists.removeNamespacePermissions(ctx.getEnvironment().getConfiguration(),
+    final Configuration conf = ctx.getEnvironment().getConfiguration();
-        namespace);
+    User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
+      @Override
+      public Void run() throws Exception {
+        AccessControlLists.removeNamespacePermissions(conf, namespace);
+        return null;
+      }
+    });
     LOG.info(namespace + "entry deleted in "+AccessControlLists.ACL_TABLE_NAME+" table.");
   }
 
@@ -1846,18 +1871,27 @@ public class AccessController extends BaseMasterAndRegionObserver
     }
   }
 
-  private AuthResult hasSomeAccess(RegionCoprocessorEnvironment e, String method, Action action) throws IOException {
+  private AuthResult hasSomeAccess(RegionCoprocessorEnvironment e, String method, Action action)
+      throws IOException {
     User requestUser = getActiveUser();
-    TableName tableName = e.getRegion().getTableDesc().getTableName();
+    final TableName tableName = e.getRegion().getTableDesc().getTableName();
-    AuthResult authResult = permissionGranted(method, requestUser,
+    AuthResult authResult = permissionGranted(method, requestUser, action, e,
-        action, e, Collections.EMPTY_MAP);
+      Collections.EMPTY_MAP);
     if (!authResult.isAllowed()) {
-      for(UserPermission userPerm:
+      final Configuration conf = e.getConfiguration();
-          AccessControlLists.getUserTablePermissions(regionEnv.getConfiguration(), tableName)) {
+      // hasSomeAccess is called from bulkload pre hooks
-        for(Action userAction: userPerm.getActions()) {
+      List<UserPermission> perms =
-          if(userAction.equals(action)) {
+        User.runAsLoginUser(new PrivilegedExceptionAction<List<UserPermission>>() {
+          @Override
+          public List<UserPermission> run() throws Exception {
+            return AccessControlLists.getUserTablePermissions(conf, tableName);
+          }
+        });
+      for (UserPermission userPerm: perms) {
+        for (Action userAction: userPerm.getActions()) {
+          if (userAction.equals(action)) {
             return AuthResult.allow(method, "Access allowed", requestUser,
-                action, tableName, null, null);
+              action, tableName, null, null);
           }
         }
       }
@@ -1932,7 +1966,7 @@ public class AccessController extends BaseMasterAndRegionObserver
   public void grant(RpcController controller,
                     AccessControlProtos.GrantRequest request,
                     RpcCallback<AccessControlProtos.GrantResponse> done) {
-    UserPermission perm = ProtobufUtil.toUserPermission(request.getUserPermission());
+    final UserPermission perm = ProtobufUtil.toUserPermission(request.getUserPermission());
     AccessControlProtos.GrantResponse response = null;
     try {
       // verify it's only running at .acl.
@@ -1952,9 +1986,17 @@ public class AccessController extends BaseMasterAndRegionObserver
             break;
           case Namespace :
             requireGlobalPermission("grant", Action.ADMIN, perm.getNamespace());
+            break;
         }
 
-        AccessControlLists.addUserPermission(regionEnv.getConfiguration(), perm);
+        User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
+          @Override
+          public Void run() throws Exception {
+            AccessControlLists.addUserPermission(regionEnv.getConfiguration(), perm);
+            return null;
+          }
+        });
+
         if (AUDITLOG.isTraceEnabled()) {
           // audit log should store permission changes in addition to auth results
           AUDITLOG.trace("Granted permission " + perm.toString());
@@ -1975,7 +2017,7 @@ public class AccessController extends BaseMasterAndRegionObserver
   public void revoke(RpcController controller,
                      AccessControlProtos.RevokeRequest request,
                      RpcCallback<AccessControlProtos.RevokeResponse> done) {
-    UserPermission perm = ProtobufUtil.toUserPermission(request.getUserPermission());
+    final UserPermission perm = ProtobufUtil.toUserPermission(request.getUserPermission());
     AccessControlProtos.RevokeResponse response = null;
     try {
       // only allowed to be called on _acl_ region
@@ -1995,9 +2037,17 @@ public class AccessController extends BaseMasterAndRegionObserver
             break;
           case Namespace :
             requireGlobalPermission("revoke", Action.ADMIN, perm.getNamespace());
+            break;
         }
 
-        AccessControlLists.removeUserPermission(regionEnv.getConfiguration(), perm);
+        User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {
+          @Override
+          public Void run() throws Exception {
+            AccessControlLists.removeUserPermission(regionEnv.getConfiguration(), perm);
+            return null;
+          }
+        });
+
         if (AUDITLOG.isTraceEnabled()) {
           // audit log should record all permission changes
           AUDITLOG.trace("Revoked permission " + perm.toString());
@@ -2026,21 +2076,32 @@ public class AccessController extends BaseMasterAndRegionObserver
           throw new CoprocessorException("AccessController not yet initialized");
         }
         List<UserPermission> perms = null;
-        if(request.getType() == AccessControlProtos.Permission.Type.Table) {
+        if (request.getType() == AccessControlProtos.Permission.Type.Table) {
-          TableName table = null;
+          final TableName table = request.hasTableName() ?
-          if (request.hasTableName()) {
+            ProtobufUtil.toTableName(request.getTableName()) : null;
-            table = ProtobufUtil.toTableName(request.getTableName());
-          }
           requirePermission("userPermissions", table, null, null, Action.ADMIN);
-
+          perms = User.runAsLoginUser(new PrivilegedExceptionAction<List<UserPermission>>() {
-          perms = AccessControlLists.getUserTablePermissions(
+            @Override
-              regionEnv.getConfiguration(), table);
+            public List<UserPermission> run() throws Exception {
+              return AccessControlLists.getUserTablePermissions(regionEnv.getConfiguration(), table);
+            }
+          });
         } else if (request.getType() == AccessControlProtos.Permission.Type.Namespace) {
-          perms = AccessControlLists.getUserNamespacePermissions(
+          final String namespace = request.getNamespaceName().toStringUtf8();
-              regionEnv.getConfiguration(), request.getNamespaceName().toStringUtf8());
+          perms = User.runAsLoginUser(new PrivilegedExceptionAction<List<UserPermission>>() {
+            @Override
+            public List<UserPermission> run() throws Exception {
+              return AccessControlLists.getUserNamespacePermissions(regionEnv.getConfiguration(),
+                namespace);
+            }
+          });
         } else {
-          perms = AccessControlLists.getUserPermissions(
+          perms = User.runAsLoginUser(new PrivilegedExceptionAction<List<UserPermission>>() {
-              regionEnv.getConfiguration(), null);
+            @Override
+            public List<UserPermission> run() throws Exception {
+              return AccessControlLists.getUserPermissions(regionEnv.getConfiguration(), null);
+            }
+          });
         }
         response = ResponseConverter.buildGetUserPermissionsResponse(perms);
       } else {

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java

@@ -28,6 +28,11 @@ import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.LargeTests;
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.HBaseAdmin;
+import org.apache.hadoop.hbase.client.HTable;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Result;
+import org.apache.hadoop.hbase.client.ResultScanner;
+import org.apache.hadoop.hbase.client.Scan;
 import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.security.access.Permission.Action;
 import org.apache.hadoop.hbase.util.Bytes;
@@ -41,7 +46,11 @@ import org.junit.experimental.categories.Category;
 @Category(LargeTests.class)
 public class TestAccessController2 extends SecureTestUtil {
 
+  private static final byte[] TEST_ROW = Bytes.toBytes("test");
   private static final byte[] TEST_FAMILY = Bytes.toBytes("f");
+  private static final byte[] TEST_QUALIFIER = Bytes.toBytes("q");
+  private static final byte[] TEST_VALUE = Bytes.toBytes("value");
+
   private static HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
   private static Configuration conf;
 
@@ -100,4 +109,102 @@ public class TestAccessController2 extends SecureTestUtil {
     assertTrue(perms.get(0).implies(Permission.Action.ADMIN));
   }
 
+  @Test
+  public void testACLTableAccess() throws Exception {
+    final Configuration conf = TEST_UTIL.getConfiguration();
+
+    // Superuser
+    User superUser = User.createUserForTesting(conf, "admin", new String[] { "supergroup" });
+
+    // Global users
+    User globalRead = User.createUserForTesting(conf, "globalRead", new String[0]);
+    User globalWrite = User.createUserForTesting(conf, "globalWrite", new String[0]);
+    User globalCreate = User.createUserForTesting(conf, "globalCreate", new String[0]);
+    User globalAdmin = User.createUserForTesting(conf, "globalAdmin", new String[0]);
+    SecureTestUtil.grantGlobal(TEST_UTIL, globalRead.getShortName(), Action.READ);
+    SecureTestUtil.grantGlobal(TEST_UTIL, globalWrite.getShortName(), Action.WRITE);
+    SecureTestUtil.grantGlobal(TEST_UTIL, globalCreate.getShortName(), Action.CREATE);
+    SecureTestUtil.grantGlobal(TEST_UTIL, globalAdmin.getShortName(), Action.ADMIN);
+
+    // Namespace users
+    User nsRead = User.createUserForTesting(conf, "nsRead", new String[0]);
+    User nsWrite = User.createUserForTesting(conf, "nsWrite", new String[0]);
+    User nsCreate = User.createUserForTesting(conf, "nsCreate", new String[0]);
+    User nsAdmin = User.createUserForTesting(conf, "nsAdmin", new String[0]);
+    SecureTestUtil.grantOnNamespace(TEST_UTIL, nsRead.getShortName(),
+      TEST_TABLE.getTableName().getNamespaceAsString(), Action.READ);
+    SecureTestUtil.grantOnNamespace(TEST_UTIL, nsWrite.getShortName(),
+      TEST_TABLE.getTableName().getNamespaceAsString(), Action.WRITE);
+    SecureTestUtil.grantOnNamespace(TEST_UTIL, nsCreate.getShortName(),
+      TEST_TABLE.getTableName().getNamespaceAsString(), Action.CREATE);
+    SecureTestUtil.grantOnNamespace(TEST_UTIL, nsAdmin.getShortName(),
+      TEST_TABLE.getTableName().getNamespaceAsString(), Action.ADMIN);
+
+    // Table users
+    User tableRead = User.createUserForTesting(conf, "tableRead", new String[0]);
+    User tableWrite = User.createUserForTesting(conf, "tableWrite", new String[0]);
+    User tableCreate = User.createUserForTesting(conf, "tableCreate", new String[0]);
+    User tableAdmin = User.createUserForTesting(conf, "tableAdmin", new String[0]);
+    SecureTestUtil.grantOnTable(TEST_UTIL, tableRead.getShortName(),
+      TEST_TABLE.getTableName(), null, null, Action.READ);
+    SecureTestUtil.grantOnTable(TEST_UTIL, tableWrite.getShortName(),
+      TEST_TABLE.getTableName(), null, null, Action.WRITE);
+    SecureTestUtil.grantOnTable(TEST_UTIL, tableCreate.getShortName(),
+      TEST_TABLE.getTableName(), null, null, Action.CREATE);
+    SecureTestUtil.grantOnTable(TEST_UTIL, tableAdmin.getShortName(),
+      TEST_TABLE.getTableName(), null, null, Action.ADMIN);
+
+    // Write tests
+
+    AccessTestAction writeAction = new AccessTestAction() {
+      @Override
+      public Object run() throws Exception {
+        HTable t = new HTable(conf, AccessControlLists.ACL_TABLE_NAME);
+        try {
+          t.put(new Put(TEST_ROW).add(AccessControlLists.ACL_LIST_FAMILY, TEST_QUALIFIER,
+            TEST_VALUE));
+          return null;
+        } finally {
+          t.close();
+        }
+      }
+    };
+
+    // All writes to ACL table denied except for GLOBAL WRITE permission and superuser
+
+    verifyDenied(writeAction, globalAdmin, globalCreate, globalRead);
+    verifyDenied(writeAction, nsAdmin, nsCreate, nsRead, nsWrite);
+    verifyDenied(writeAction, tableAdmin, tableCreate, tableRead, tableWrite);
+    verifyAllowed(writeAction, superUser, globalWrite);
+
+    // Read tests
+
+    AccessTestAction scanAction = new AccessTestAction() {
+      @Override
+      public Object run() throws Exception {
+        HTable t = new HTable(conf, AccessControlLists.ACL_TABLE_NAME);
+        try {
+          ResultScanner s = t.getScanner(new Scan());
+          try {
+            for (Result r = s.next(); r != null; r = s.next()) {
+              // do nothing
+            }
+          } finally {
+            s.close();
+          }
+          return null;
+        } finally {
+          t.close();
+        }
+      }
+    };
+
+    // All reads from ACL table denied except for GLOBAL READ and superuser
+
+    verifyDenied(scanAction, globalAdmin, globalCreate, globalWrite);
+    verifyDenied(scanAction, nsCreate, nsAdmin, nsRead, nsWrite);
+    verifyDenied(scanAction, tableCreate, tableAdmin, tableRead, tableWrite);
+    verifyAllowed(scanAction, superUser, globalRead);
+  }
+
 }