HBASE-19401 Add missing security checks in RSRpcServices
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/991e163c Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/991e163c Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/991e163c Branch: refs/heads/branch-2 Commit: 991e163cc271c691b5d0d9a871cb2ba75b7ba337 Parents: 161f9de Author: Apekshit Sharma <a...@apache.org> Authored: Tue Feb 13 12:33:43 2018 -0800 Committer: Apekshit Sharma <a...@apache.org> Committed: Thu Feb 22 16:23:47 2018 -0800 ---------------------------------------------------------------------- .../hbase/regionserver/RSRpcServices.java | 22 +++++++- .../access/TestAdminOnlyOperations.java | 56 ++++++++++++++------ 2 files changed, 60 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/991e163c/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java index 3efe0c6..33ee548 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java @@ -498,6 +498,24 @@ public class RSRpcServices implements HBaseRPCErrorHandler, } /** + * 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. * @param nonceGroup Nonce group from the request. @@ -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); } http://git-wip-us.apache.org/repos/asf/hbase/blob/991e163c/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAdminOnlyOperations.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAdminOnlyOperations.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAdminOnlyOperations.java index d4b0650..42d2f36 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAdminOnlyOperations.java +++ b/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<?>) () -> { - 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; - }); + verifiedDeniedServiceException(USER_NON_ADMIN, action); } @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); + } }
