HBASE-19401 Add missing security checks in RSRpcServices

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java

@@ -497,6 +497,24 @@ public class RSRpcServices implements HBaseRPCErrorHandler,
     return builder.setIndex(index).build();
   }
 
+  /**
+   * Checks for the following pre-checks in order:
+   * <ol>
+   *   <li>RegionServer is running</li>
+   *   <li>If authorization is enabled, then RPC caller has ADMIN permissions</li>
+   * </ol>
+   * @param requestName name of rpc request. Used in reporting failures to provide context.
+   * @throws ServiceException If any of the above listed pre-check fails.
+   */
+  private void rpcPreCheck(String requestName) throws ServiceException {
+    try {
+      checkOpen();
+      requirePermission(requestName, Permission.Action.ADMIN);
+    } catch (IOException ioe) {
+      throw new ServiceException(ioe);
+    }
+  }
+
   /**
    * Starts the nonce operation for a mutation, if needed.
    * @param mutation Mutation.
@@ -1438,9 +1456,8 @@ public class RSRpcServices implements HBaseRPCErrorHandler,
 
   /**
    * Called to verify that this server is up and running.
-   *
-   * @throws IOException
    */
+  // TODO : Rename this and HMaster#checkInitialized to isRunning() (or a better name).
   protected void checkOpen() throws IOException {
     if (regionServer.isAborted()) {
       throw new RegionServerAbortedException("Server " + regionServer.serverName + " aborting");
@@ -3432,6 +3449,7 @@ public class RSRpcServices implements HBaseRPCErrorHandler,
   @Override
   public CoprocessorServiceResponse execRegionServerService(RpcController controller,
       CoprocessorServiceRequest request) throws ServiceException {
+    rpcPreCheck("execRegionServerService");
     return regionServer.execRegionServerService(controller, request);
   }
 

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

@@ -1,3 +1,4 @@
+
 /**
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
@@ -31,12 +32,14 @@ import java.util.HashMap;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.ConnectionFactory;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
 import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor;
+import org.apache.hadoop.hbase.coprocessor.RegionServerCoprocessor;
 import org.apache.hadoop.hbase.ipc.protobuf.generated.TestProtos;
 import org.apache.hadoop.hbase.ipc.protobuf.generated.TestRpcServiceProtos;
 import org.apache.hadoop.hbase.security.AccessDeniedException;
@@ -88,7 +91,7 @@ public class TestAdminOnlyOperations {
   private static User USER_GROUP_ADMIN;
 
   // Dummy service to test execService calls. Needs to be public so can be loaded as Coprocessor.
-  public static class DummyCpService implements MasterCoprocessor {
+  public static class DummyCpService implements MasterCoprocessor, RegionServerCoprocessor {
     public DummyCpService() {}
 
     @Override
@@ -103,7 +106,8 @@ public class TestAdminOnlyOperations {
     conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, AccessController.class.getName() +
       "," + DummyCpService.class.getName());
     conf.set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, AccessController.class.getName());
-    conf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, AccessController.class.getName());
+    conf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, AccessController.class.getName() +
+      "," + DummyCpService.class.getName());
     conf.set(User.HBASE_SECURITY_AUTHORIZATION_CONF_KEY, "true");
     SecureTestUtil.configureSuperuser(conf);
   }
@@ -161,6 +165,24 @@ public class TestAdminOnlyOperations {
     });
   }
 
+  private void verifiedDeniedServiceException(User user, Action action) throws Exception {
+    user.runAs((PrivilegedExceptionAction<?>) () -> {
+      boolean accessDenied = false;
+      try (Connection conn = ConnectionFactory.createConnection(conf);
+          Admin admin = conn.getAdmin()) {
+        action.run(admin);
+      } catch (ServiceException e) {
+        // For MasterRpcServices.execService.
+        if (e.getCause() instanceof AccessDeniedException) {
+          accessDenied = true;
+        }
+      }
+      assertTrue("Expected access to be denied", accessDenied);
+      return null;
+    });
+
+  }
+
   private void verifyAdminCheckForAction(Action action) throws Exception {
     verifyAllowed(USER_ADMIN, action);
     verifyAllowed(USER_GROUP_ADMIN, action);
@@ -207,20 +229,7 @@ public class TestAdminOnlyOperations {
     verifyAllowed(USER_ADMIN, action);
     verifyAllowed(USER_GROUP_ADMIN, action);
     // This is same as above verifyAccessDenied
-    USER_NON_ADMIN.runAs((PrivilegedExceptionAction<?>) () -> {
+    verifiedDeniedServiceException(USER_NON_ADMIN, action);
-      boolean accessDenied = false;
-      try (Connection conn = ConnectionFactory.createConnection(conf);
-          Admin admin = conn.getAdmin()) {
-        action.run(admin);
-      } catch (ServiceException e) {
-        // For MasterRpcServices.execService.
-        if (e.getCause() instanceof AccessDeniedException) {
-          accessDenied = true;
-        }
-      }
-      assertTrue("Expected access to be denied", accessDenied);
-      return null;
-    });
   }
 
   @Test
@@ -241,4 +250,19 @@ public class TestAdminOnlyOperations {
   public void testSetNormalizerRunning() throws Exception {
     verifyAdminCheckForAction((admin) -> admin.normalizerSwitch(true));
   }
+
+  @Test
+  public void testExecRegionServerService() throws Exception {
+    Action action = (admin) -> {
+      ServerName serverName = TEST_UTIL.getHBaseCluster().getRegionServer(0).getServerName();
+      TestRpcServiceProtos.TestProtobufRpcProto.BlockingInterface service =
+          TestRpcServiceProtos.TestProtobufRpcProto.newBlockingStub(
+              admin.coprocessorService(serverName));
+      service.ping(null, TestProtos.EmptyRequestProto.getDefaultInstance());
+    };
+
+    verifyAllowed(USER_ADMIN, action);
+    verifyAllowed(USER_GROUP_ADMIN, action);
+    verifiedDeniedServiceException(USER_NON_ADMIN, action);
+  }
 }