From ad425e8603fe07e45f9f00890cc4f063346ddc9f Mon Sep 17 00:00:00 2001 From: Apekshit Sharma Date: Tue, 13 Mar 2018 17:13:56 +0530 Subject: [PATCH] HBASE-20185 Fix ACL check for MasterRpcServices#execProcedure --- .../hbase/master/MasterRpcServices.java | 4 ++- .../master/snapshot/SnapshotManager.java | 8 +++++ .../procedure/MasterProcedureManager.java | 31 +++++++++++-------- .../MasterFlushTableProcedureManager.java | 10 ++++++ .../hbase/regionserver/RSRpcServices.java | 2 +- .../security/access/AccessController.java | 3 ++ .../SimpleMasterProcedureManager.java | 6 ++++ 7 files changed, 49 insertions(+), 15 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index 8f92041f6bc..70f4cd42cc6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -57,6 +57,7 @@ import org.apache.hadoop.hbase.io.hfile.HFile; import org.apache.hadoop.hbase.ipc.CoprocessorRpcUtils; import org.apache.hadoop.hbase.ipc.PriorityFunction; import org.apache.hadoop.hbase.ipc.QosPriority; +import org.apache.hadoop.hbase.ipc.RpcServer; import org.apache.hadoop.hbase.ipc.RpcServer.BlockingServiceAndInterface; import org.apache.hadoop.hbase.ipc.RpcServerFactory; import org.apache.hadoop.hbase.ipc.RpcServerInterface; @@ -831,8 +832,8 @@ public class MasterRpcServices extends RSRpcServices @Override public ExecProcedureResponse execProcedure(RpcController controller, ExecProcedureRequest request) throws ServiceException { - rpcPreCheck("execProcedure"); try { + master.checkInitialized(); ProcedureDescription desc = request.getProcedure(); MasterProcedureManager mpm = master.getMasterProcedureManagerHost().getProcedureManager( desc.getSignature()); @@ -841,6 +842,7 @@ public class MasterRpcServices extends RSRpcServices + desc.getSignature())); } LOG.info(master.getClientIdAuditPrefix() + " procedure request for: " + desc.getSignature()); + mpm.checkPermissions(desc, accessChecker, RpcServer.getRequestUser().orElse(null)); mpm.execProcedure(desc); // send back the max amount of time the client should wait for the procedure // to complete diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java index 3870601db43..1abd605f990 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java @@ -63,6 +63,7 @@ import org.apache.hadoop.hbase.procedure.ZKProcedureCoordinator; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.security.AccessDeniedException; import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.AccessChecker; import org.apache.hadoop.hbase.snapshot.ClientSnapshotDescriptionUtils; import org.apache.hadoop.hbase.snapshot.HBaseSnapshotException; import org.apache.hadoop.hbase.snapshot.RestoreSnapshotException; @@ -1149,6 +1150,13 @@ public class SnapshotManager extends MasterProcedureManager implements Stoppable takeSnapshot(toSnapshotDescription(desc)); } + @Override + public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user) + throws IOException { + // Done by AccessController as part of preSnapshot coprocessor hook (legacy code path). + // In future, when we AC is removed for good, that check should be moved here. + } + @Override public boolean isProcedureDone(ProcedureDescription desc) throws IOException { return isSnapshotDone(toSnapshotDescription(desc)); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/MasterProcedureManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/MasterProcedureManager.java index 89e55d136df..b705157d24a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/MasterProcedureManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/MasterProcedureManager.java @@ -19,6 +19,8 @@ package org.apache.hadoop.hbase.procedure; import java.io.IOException; +import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.AccessChecker; import org.apache.yetus.audience.InterfaceAudience; import org.apache.yetus.audience.InterfaceStability; import org.apache.hadoop.hbase.Stoppable; @@ -34,28 +36,26 @@ import org.apache.zookeeper.KeeperException; * * To implement a custom globally barriered procedure, user needs to extend two classes: * {@link MasterProcedureManager} and {@link RegionServerProcedureManager}. Implementation of -* {@link MasterProcedureManager} is loaded into {@link org.apache.hadoop.hbase.master.HMaster} +* {@link MasterProcedureManager} is loaded into {@link org.apache.hadoop.hbase.master.HMaster} * process via configuration parameter 'hbase.procedure.master.classes', while implementation of -* {@link RegionServerProcedureManager} is loaded into +* {@link RegionServerProcedureManager} is loaded into * {@link org.apache.hadoop.hbase.regionserver.HRegionServer} process via * configuration parameter 'hbase.procedure.regionserver.classes'. * -* An example of globally barriered procedure implementation is +* An example of globally barriered procedure implementation is * {@link org.apache.hadoop.hbase.master.snapshot.SnapshotManager} and * {@link org.apache.hadoop.hbase.regionserver.snapshot.RegionServerSnapshotManager}. * * A globally barriered procedure is identified by its signature (usually it is the name of the * procedure znode). During the initialization phase, the initialize methods are called by both -* {@link org.apache.hadoop.hbase.master.HMaster} -* and {@link org.apache.hadoop.hbase.regionserver.HRegionServer} which create the procedure znode -* and register the listeners. A procedure can be triggered by its signature and an instant name -* (encapsulated in a {@link ProcedureDescription} object). When the servers are shutdown, +* {@link org.apache.hadoop.hbase.master.HMaster} +* and {@link org.apache.hadoop.hbase.regionserver.HRegionServer} which create the procedure znode +* and register the listeners. A procedure can be triggered by its signature and an instant name +* (encapsulated in a {@link ProcedureDescription} object). When the servers are shutdown, * the stop methods on both classes are called to clean up the data associated with the procedure. */ @InterfaceAudience.Private -@InterfaceStability.Evolving -public abstract class MasterProcedureManager extends ProcedureManager implements - Stoppable { +public abstract class MasterProcedureManager extends ProcedureManager implements Stoppable { /** * Initialize a globally barriered procedure for master. * @@ -73,9 +73,7 @@ public abstract class MasterProcedureManager extends ProcedureManager implements * @param desc Procedure description * @throws IOException */ - public void execProcedure(ProcedureDescription desc) throws IOException { - - } + public void execProcedure(ProcedureDescription desc) throws IOException {} /** * Execute a distributed procedure on cluster with return data. @@ -89,6 +87,13 @@ public abstract class MasterProcedureManager extends ProcedureManager implements return null; } + /** + * Check for required permissions before executing the procedure. + * @throws IOException if permissions requirements are not met. + */ + public abstract void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, + User user) throws IOException; + /** * Check if the procedure is finished successfully * diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/MasterFlushTableProcedureManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/MasterFlushTableProcedureManager.java index 75e041a15df..381e526456d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/MasterFlushTableProcedureManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/MasterFlushTableProcedureManager.java @@ -41,6 +41,9 @@ import org.apache.hadoop.hbase.procedure.Procedure; import org.apache.hadoop.hbase.procedure.ProcedureCoordinator; import org.apache.hadoop.hbase.procedure.ProcedureCoordinatorRpcs; import org.apache.hadoop.hbase.procedure.ZKProcedureCoordinator; +import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.AccessChecker; +import org.apache.hadoop.hbase.security.access.Permission; import org.apache.hadoop.hbase.util.Pair; import org.apache.hadoop.hbase.zookeeper.MetaTableLocator; import org.apache.yetus.audience.InterfaceAudience; @@ -181,6 +184,13 @@ public class MasterFlushTableProcedureManager extends MasterProcedureManager { monitor.rethrowException(); } + @Override + public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user) + throws IOException { + // Done by AccessController as part of preTableFlush coprocessor hook (legacy code path). + // In future, when we AC is removed for good, that check should be moved here. + } + @Override public synchronized boolean isProcedureDone(ProcedureDescription desc) throws IOException { // Procedure instance name is the table name. 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 acf25cff647..51109393bc0 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 @@ -331,7 +331,7 @@ public class RSRpcServices implements HBaseRPCErrorHandler, // Initialized in start() since AccessChecker needs ZKWatcher which is created by HRegionServer // after RSRpcServices constructor and before start() is called. // Initialized only if authorization is enabled, else remains null. - private AccessChecker accessChecker; + protected AccessChecker accessChecker; /** * Services launched in RSRpcServices. By default they are on but you can use the below diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index b20d7e49962..cfc0b914a04 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -1136,6 +1136,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, public void preSnapshot(final ObserverContext ctx, final SnapshotDescription snapshot, final TableDescriptor hTableDescriptor) throws IOException { + // Move this ACL check to SnapshotManager#checkPermissions as part of AC deprecation. requirePermission(ctx, "snapshot " + snapshot.getName(), hTableDescriptor.getTableName(), null, null, Permission.Action.ADMIN); } @@ -1266,6 +1267,8 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, @Override public void preTableFlush(final ObserverContext ctx, final TableName tableName) throws IOException { + // Move this ACL check to MasterFlushTableProcedureManager#checkPermissions as part of AC + // deprecation. requirePermission(ctx, "flushTable", tableName, null, null, Action.ADMIN, Action.CREATE); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleMasterProcedureManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleMasterProcedureManager.java index 579fcd3ab0d..e1d46ef92ee 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleMasterProcedureManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleMasterProcedureManager.java @@ -29,6 +29,8 @@ import org.apache.hadoop.hbase.errorhandling.ForeignExceptionDispatcher; import org.apache.hadoop.hbase.master.MasterServices; import org.apache.hadoop.hbase.master.MetricsMaster; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.ProcedureDescription; +import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.AccessChecker; import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -113,6 +115,10 @@ public class SimpleMasterProcedureManager extends MasterProcedureManager { return returnData.values().iterator().next(); } + @Override + public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user) + throws IOException {} + @Override public boolean isProcedureDone(ProcedureDescription desc) throws IOException { return done;