HBASE-8692. [AccessController] Restrict HTableDescriptor enumeration

git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1496238 13f79535-47bb-0310-9956-ffa450edef68

hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java

@@ -591,19 +591,14 @@ public class HBaseAdmin implements Abortable, Closeable {
           MasterMonitorKeepAliveConnection master = connection.getKeepAliveMasterMonitorService();
           try {
             GetTableDescriptorsRequest req =
-              RequestConverter.buildGetTableDescriptorsRequest(null);
+              RequestConverter.buildGetTableDescriptorsRequest(tableName);
             htds = master.getTableDescriptors(null, req);
           } catch (ServiceException se) {
             throw ProtobufUtil.getRemoteException(se);
           } finally {
             master.close();
           }
-          for (TableSchema ts : htds.getTableSchemaList()) {
+          tableExists = !htds.getTableSchemaList().isEmpty();
-            if (Bytes.equals(tableName, ts.getName().toByteArray())) {
-              tableExists = true;
-              break;
-            }
-          }
           if (!tableExists) {
             break;
           }

hbase-client/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java

@@ -2309,7 +2309,7 @@ public class HConnectionManager {
       MasterMonitorKeepAliveConnection master = getKeepAliveMasterMonitorService();
       try {
         GetTableDescriptorsRequest req =
-          RequestConverter.buildGetTableDescriptorsRequest(null);
+          RequestConverter.buildGetTableDescriptorsRequest((List<String>)null);
         return ProtobufUtil.getHTableDescriptorArray(master.getTableDescriptors(null, req));
       } catch (ServiceException se) {
         throw ProtobufUtil.getRemoteException(se);
@@ -2351,17 +2351,15 @@ public class HConnectionManager {
       GetTableDescriptorsResponse htds;
       try {
         GetTableDescriptorsRequest req =
-          RequestConverter.buildGetTableDescriptorsRequest(null);
+          RequestConverter.buildGetTableDescriptorsRequest(tableName);
         htds = master.getTableDescriptors(null, req);
       } catch (ServiceException se) {
         throw ProtobufUtil.getRemoteException(se);
       } finally {
         master.close();
       }
-      for (TableSchema ts : htds.getTableSchemaList()) {
+      if (!htds.getTableSchemaList().isEmpty()) {
-        if (Bytes.equals(tableName, ts.getName().toByteArray())) {
+        return HTableDescriptor.convert(htds.getTableSchemaList().get(0));
-          return HTableDescriptor.convert(ts);
-        }
       }
       throw new TableNotFoundException(Bytes.toString(tableName));
     }

hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java

@@ -1114,6 +1114,19 @@ public final class RequestConverter {
     return builder.build();
   }
 
+  /**
+   * Creates a protocol buffer GetTableDescriptorsRequest for a single table
+   *
+   * @param tableName the table name
+   * @return a GetTableDescriptorsRequest
+   */
+  public static GetTableDescriptorsRequest buildGetTableDescriptorsRequest(
+      final byte[] tableName) {
+    return GetTableDescriptorsRequest.newBuilder()
+      .addTableNames(Bytes.toString(tableName))
+      .build();
+  }
+
   /**
    * Creates a protocol buffer IsMasterRunningRequest
    *

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseMasterObserver.java

@@ -343,4 +343,15 @@ public class BaseMasterObserver implements MasterObserver {
   public void postDeleteSnapshot(final ObserverContext<MasterCoprocessorEnvironment> ctx,
       final SnapshotDescription snapshot) throws IOException {
   }
+
+  @Override
+  public void preGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<String> tableNamesList, List<HTableDescriptor> descriptors) throws IOException {
+  }
+
+  @Override
+  public void postGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<HTableDescriptor> descriptors) throws IOException {
+  }
+
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java

@@ -610,4 +610,23 @@ public interface MasterObserver extends Coprocessor {
    */
   void postDeleteSnapshot(final ObserverContext<MasterCoprocessorEnvironment> ctx,
       final SnapshotDescription snapshot) throws IOException;
+
+  /**
+   * Called before a getTableDescriptors request has been processed.
+   * @param ctx the environment to interact with the framework and master
+   * @param tableNamesList the list of table names, or null if querying for all
+   * @param descriptors an empty list, can be filled with what to return if bypassing
+   * @throws IOException
+   */
+  void preGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<String> tableNamesList, List<HTableDescriptor> descriptors) throws IOException;
+
+  /**
+   * Called after a getTableDescriptors request has been processed.
+   * @param ctx the environment to interact with the framework and master
+   * @param descriptors the list of descriptors about to be returned
+   * @throws IOException
+   */
+  void postGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<HTableDescriptor> descriptors) throws IOException;
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -1426,6 +1426,7 @@ MasterServices, Server {
     SYNC,
     ASYNC
   }
+
   /**
    * Assigns balancer switch according to BalanceSwitchMode
    * @param b new balancer switch
@@ -2409,33 +2410,55 @@ MasterServices, Server {
    */
   public GetTableDescriptorsResponse getTableDescriptors(
 	      RpcController controller, GetTableDescriptorsRequest req) throws ServiceException {
-    GetTableDescriptorsResponse.Builder builder = GetTableDescriptorsResponse.newBuilder();
+    List<HTableDescriptor> descriptors = new ArrayList<HTableDescriptor>();
+
+    boolean bypass = false;
+    if (this.cpHost != null) {
+      try {
+        bypass = this.cpHost.preGetTableDescriptors(req.getTableNamesList(), descriptors);
+      } catch (IOException ioe) {
+        throw new ServiceException(ioe);
+      }
+    }
+
+    if (!bypass) {
       if (req.getTableNamesCount() == 0) {
         // request for all TableDescriptors
-      Map<String, HTableDescriptor> descriptors = null;
+        Map<String, HTableDescriptor> descriptorMap = null;
         try {
-        descriptors = this.tableDescriptors.getAll();
+          descriptorMap = this.tableDescriptors.getAll();
         } catch (IOException e) {
           LOG.warn("Failed getting all descriptors", e);
         }
-      if (descriptors != null) {
+        if (descriptorMap != null) {
-        for (HTableDescriptor htd : descriptors.values()) {
+          descriptors.addAll(descriptorMap.values());
-          builder.addTableSchema(htd.convert());
         }
-      }
+      } else {
-    }
-    else {
         for (String s: req.getTableNamesList()) {
-        HTableDescriptor htd = null;
           try {
-          htd = this.tableDescriptors.get(s);
+            HTableDescriptor desc = this.tableDescriptors.get(s);
+            if (desc != null) {
+              descriptors.add(desc);
+            }
           } catch (IOException e) {
             LOG.warn("Failed getting descriptor for " + s, e);
           }
-        if (htd == null) continue;
-        builder.addTableSchema(htd.convert());
         }
       }
+
+      if (this.cpHost != null) {
+        try {
+          this.cpHost.postGetTableDescriptors(descriptors);
+        } catch (IOException ioe) {
+          throw new ServiceException(ioe);
+        }
+      }
+    }
+
+    GetTableDescriptorsResponse.Builder builder = GetTableDescriptorsResponse.newBuilder();
+    for (HTableDescriptor htd: descriptors) {
+      builder.addTableSchema(htd.convert());
+    }
     return builder.build();
   }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java

@@ -1113,4 +1113,44 @@ public class MasterCoprocessorHost
       }
     }
   }
+
+  public boolean preGetTableDescriptors(final List<String> tableNamesList,
+      final List<HTableDescriptor> descriptors) throws IOException {
+    boolean bypass = false;
+    ObserverContext<MasterCoprocessorEnvironment> ctx = null;
+    for (MasterEnvironment env : coprocessors) {
+      if (env.getInstance() instanceof MasterObserver) {
+        ctx = ObserverContext.createAndPrepare(env, ctx);
+        try {
+          ((MasterObserver) env.getInstance()).preGetTableDescriptors(ctx,
+            tableNamesList, descriptors);
+        } catch (Throwable e) {
+          handleCoprocessorThrowable(env, e);
+        }
+        bypass |= ctx.shouldBypass();
+        if (ctx.shouldComplete()) {
+          break;
+        }
+      }
+    }
+    return bypass;
+  }
+
+  public void postGetTableDescriptors(List<HTableDescriptor> descriptors) throws IOException {
+    ObserverContext<MasterCoprocessorEnvironment> ctx = null;
+    for (MasterEnvironment env: coprocessors) {
+      if (env.getInstance() instanceof MasterObserver) {
+        ctx = ObserverContext.createAndPrepare(env, ctx);
+        try {
+          ((MasterObserver)env.getInstance()).postGetTableDescriptors(ctx, descriptors);
+        } catch (Throwable e) {
+          handleCoprocessorThrowable(env, e);
+        }
+        if (ctx.shouldComplete()) {
+          break;
+        }
+      }
+    }
+  }
+
 }

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

@@ -49,10 +49,13 @@ import org.apache.hadoop.hbase.client.Scan;
 import org.apache.hadoop.hbase.client.Durability;
 import org.apache.hadoop.hbase.coprocessor.*;
 import org.apache.hadoop.hbase.exceptions.CoprocessorException;
+import org.apache.hadoop.hbase.exceptions.TableNotDisabledException;
+import org.apache.hadoop.hbase.exceptions.TableNotFoundException;
 import org.apache.hadoop.hbase.filter.CompareFilter;
 import org.apache.hadoop.hbase.filter.FilterList;
 import org.apache.hadoop.hbase.filter.ByteArrayComparable;
 import org.apache.hadoop.hbase.ipc.RequestContext;
+import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.master.RegionPlan;
 import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.protobuf.ResponseConverter;
@@ -1361,4 +1364,38 @@ public class AccessController extends BaseRegionObserver
     familyMap.put(family, qualifier != null ? ImmutableSet.of(qualifier) : null);
     return familyMap;
   }
+
+  @Override
+  public void preGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<String> tableNamesList, List<HTableDescriptor> descriptors) throws IOException {
+    // If the list is empty, this is a request for all table descriptors and requires GLOBAL
+    // ADMIN privs.
+    if (tableNamesList == null || tableNamesList.isEmpty()) {
+      requireGlobalPermission("getTableDescriptors", Permission.Action.ADMIN, null, null);
+    }
+    // Otherwise, if the requestor has ADMIN or CREATE privs for all listed tables, the
+    // request can be granted.
+    else {
+      MasterServices masterServices = ctx.getEnvironment().getMasterServices();
+      for (String tableName: tableNamesList) {
+        // Do not deny if the table does not exist
+        byte[] nameAsBytes = Bytes.toBytes(tableName);
+        try {
+          masterServices.checkTableModifiable(nameAsBytes);
+        } catch (TableNotFoundException ex) {
+          // Skip checks for a table that does not exist
+          continue;
+        } catch (TableNotDisabledException ex) {
+          // We don't care about this
+        }
+        requirePermission("getTableDescriptors", nameAsBytes, null, null,
+          Permission.Action.ADMIN, Permission.Action.CREATE);
+      }
+    }
+  }
+
+  @Override
+  public void postGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+      List<HTableDescriptor> descriptors) throws IOException {
+  }
 }

hbase-server/src/main/ruby/hbase/admin.rb

@@ -319,15 +319,7 @@ module Hbase
     #----------------------------------------------------------------------------------------------
     # Returns table's structure description
     def describe(table_name)
-      tables = @admin.listTables.to_a
+      @admin.getTableDescriptor(table_name.to_java_bytes).to_s
-      tables << org.apache.hadoop.hbase.HTableDescriptor::META_TABLEDESC
-
-      tables.each do |t|
-        # Found the table
-        return t.to_s if t.getNameAsString == table_name
-      end
-
-      raise(ArgumentError, "Failed to find table named #{table_name}")
     end
 
     #----------------------------------------------------------------------------------------------

hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java

@@ -31,8 +31,6 @@ import java.util.Map;
 import java.util.NavigableMap;
 import java.util.concurrent.CountDownLatch;
 
-import junit.framework.Assert;
-
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
@@ -45,6 +43,7 @@ import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
 import org.apache.hadoop.hbase.master.RegionPlan;
 import org.apache.hadoop.hbase.master.RegionState;
 import org.apache.hadoop.hbase.protobuf.generated.HBaseProtos.SnapshotDescription;
+import org.apache.hadoop.hbase.protobuf.generated.MasterMonitorProtos.GetTableDescriptorsRequest;
 import org.apache.hadoop.hbase.protobuf.RequestConverter;
 import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
@@ -125,6 +124,8 @@ public class TestMasterObserver {
     private boolean postDisableTableHandlerCalled;
     private boolean preModifyTableHandlerCalled;
     private boolean postModifyTableHandlerCalled;
+    private boolean preGetTableDescriptorsCalled;
+    private boolean postGetTableDescriptorsCalled;
 
     public void enableBypass(boolean bypass) {
       this.bypass = bypass;
@@ -183,6 +184,10 @@ public class TestMasterObserver {
       postEnableTableHandlerCalled = false;
       preDisableTableHandlerCalled = false;
       postDisableTableHandlerCalled = false;
+      preModifyTableHandlerCalled = false;
+      postModifyTableHandlerCalled = false;
+      preGetTableDescriptorsCalled = false;
+      postGetTableDescriptorsCalled = false;
     }
 
     @Override
@@ -818,6 +823,22 @@ public class TestMasterObserver {
     public boolean preDisableTableHandlerCalledOnly() {
       return preDisableTableHandlerCalled && !postDisableTableHandlerCalled;
     }
+
+    @Override
+    public void preGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+        List<String> tableNamesList, List<HTableDescriptor> descriptors) throws IOException {
+      preGetTableDescriptorsCalled = true;
+    }
+
+    @Override
+    public void postGetTableDescriptors(ObserverContext<MasterCoprocessorEnvironment> ctx,
+        List<HTableDescriptor> descriptors) throws IOException {
+      postGetTableDescriptorsCalled = true;
+    }
+
+    public boolean wasGetTableDescriptorsCalled() {
+      return preGetTableDescriptorsCalled && postGetTableDescriptorsCalled;
+    }
   }
 
   private static HBaseTestingUtility UTIL = new HBaseTestingUtility();
@@ -1196,5 +1217,22 @@ public class TestMasterObserver {
     }
   }
 
+  @Test
+  public void testTableDescriptorsEnumeration() throws Exception {
+    MiniHBaseCluster cluster = UTIL.getHBaseCluster();
+
+    HMaster master = cluster.getMaster();
+    MasterCoprocessorHost host = master.getCoprocessorHost();
+    CPMasterObserver cp = (CPMasterObserver)host.findCoprocessor(
+        CPMasterObserver.class.getName());
+    cp.resetStates();
+
+    GetTableDescriptorsRequest req =
+        RequestConverter.buildGetTableDescriptorsRequest((List<String>)null);
+    master.getTableDescriptors(null, req);
+
+    assertTrue("Coprocessor should be called on table descriptors request",
+      cp.wasGetTableDescriptorsCalled());
   }
 
+}

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

@@ -78,6 +78,7 @@ import org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost;
 import org.apache.hadoop.hbase.regionserver.RegionServerCoprocessorHost;
 import org.apache.hadoop.hbase.regionserver.ScanType;
 import org.apache.hadoop.hbase.exceptions.AccessDeniedException;
+import org.apache.hadoop.hbase.exceptions.TableNotFoundException;
 import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.security.access.AccessControlLists;
 import org.apache.hadoop.hbase.security.access.Permission;
@@ -223,7 +224,12 @@ public class TestAccessController {
   @After
   public void tearDown() throws Exception {
     // Clean the _acl_ table
+    try {
       TEST_UTIL.deleteTable(TEST_TABLE.getTableName());
+    } catch (TableNotFoundException ex) {
+      // Test deleted the table, no problem
+      LOG.info("Test deleted table " + Bytes.toString(TEST_TABLE.getTableName()));
+    }
     assertEquals(0, AccessControlLists.getTablePermissions(conf, TEST_TABLE.getTableName()).size());
   }
 
@@ -1980,4 +1986,85 @@ public class TestAccessController {
       table.close();
     }
   }
+
+  @Test
+  public void testTableDescriptorsEnumeration() throws Exception {
+    User TABLE_ADMIN = User.createUserForTesting(conf, "UserA", new String[0]);
+
+    // Grant TABLE ADMIN privs
+    HTable acl = new HTable(conf, AccessControlLists.ACL_TABLE_NAME);
+    try {
+      BlockingRpcChannel service = acl.coprocessorService(TEST_TABLE.getTableName());
+      AccessControlService.BlockingInterface protocol =
+        AccessControlService.newBlockingStub(service);
+      ProtobufUtil.grant(protocol, TABLE_ADMIN.getShortName(), TEST_TABLE.getTableName(),
+        null, null, Permission.Action.ADMIN);
+    } finally {
+      acl.close();
+    }
+
+    PrivilegedExceptionAction listTablesAction = new PrivilegedExceptionAction() {
+      public Object run() throws Exception {
+        HBaseAdmin admin = new HBaseAdmin(TEST_UTIL.getConfiguration());
+        try {
+          admin.listTables();
+        } finally {
+          admin.close();
+        }
+        return null;
+      }
+    };
+
+    PrivilegedExceptionAction getTableDescAction = new PrivilegedExceptionAction() {
+      public Object run() throws Exception {
+        HBaseAdmin admin = new HBaseAdmin(TEST_UTIL.getConfiguration());
+        try {
+          admin.getTableDescriptor(TEST_TABLE.getTableName());
+        } finally {
+          admin.close();
+        }
+        return null;
+      }
+    };
+
+    verifyAllowed(listTablesAction, SUPERUSER, USER_ADMIN);
+    verifyDenied(listTablesAction, USER_CREATE, USER_RW, USER_RO, USER_NONE, TABLE_ADMIN);
+
+    verifyAllowed(getTableDescAction, SUPERUSER, USER_ADMIN, USER_CREATE, TABLE_ADMIN);
+    verifyDenied(getTableDescAction, USER_RW, USER_RO, USER_NONE);
+  }
+
+  @Test
+  public void testTableDeletion() throws Exception {
+    User TABLE_ADMIN = User.createUserForTesting(conf, "TestUser", new String[0]);
+
+    // Grant TABLE ADMIN privs
+    HTable acl = new HTable(conf, AccessControlLists.ACL_TABLE_NAME);
+    try {
+      BlockingRpcChannel service = acl.coprocessorService(TEST_TABLE.getTableName());
+      AccessControlService.BlockingInterface protocol =
+        AccessControlService.newBlockingStub(service);
+      ProtobufUtil.grant(protocol, TABLE_ADMIN.getShortName(), TEST_TABLE.getTableName(),
+        null, null, Permission.Action.ADMIN);
+    } finally {
+      acl.close();
+    }
+
+    PrivilegedExceptionAction deleteTableAction = new PrivilegedExceptionAction() {
+      public Object run() throws Exception {
+        HBaseAdmin admin = new HBaseAdmin(TEST_UTIL.getConfiguration());
+        try {
+          admin.disableTable(TEST_TABLE.getTableName());
+          admin.deleteTable(TEST_TABLE.getTableName());
+        } finally {
+          admin.close();
+        }
+        return null;
+      }
+    };
+
+    verifyDenied(deleteTableAction, USER_RW, USER_RO, USER_NONE);
+    verifyAllowed(deleteTableAction, TABLE_ADMIN);
+  }
+
 }

hbase-server/src/test/ruby/hbase/admin_test.rb

@@ -191,7 +191,7 @@ module Hbase
     #-------------------------------------------------------------------------------
 
     define_test "describe should fail for non-existent tables" do
-      assert_raise(ArgumentError) do
+      assert_raise(NativeException) do
         admin.describe('.NOT.EXISTS.')
       end
     end