From f4380a4891e4093a38687e9b8135773b8c65fe81 Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Mon, 31 Aug 2026 21:24:02 +0800 Subject: [PATCH] HBASE-30337 Miscellaneous improvements on rpc checks (#8560) Signed-off-by: Viraj Jasani Signed-off-by: Xiao Liu Reviewed-by: Aman Poonia (cherry picked from commit fb4286d3046ff525e8ffca8cb7fec1d2fb45b9f0) --- .../security/access/AccessControlUtil.java | 16 ++ .../hbase/security/access/Permission.java | 5 +- .../access/ShadedAccessControlUtil.java | 16 ++ .../hbase/coprocessor/MasterObserver.java | 52 +++- .../hbase/master/MasterCoprocessorHost.java | 32 ++- .../hbase/master/MasterRpcServices.java | 18 +- .../hbase/regionserver/RSRpcServices.java | 6 +- .../security/access/AccessController.java | 38 ++- .../security/access/TestAccessController.java | 62 +++++ .../TestAccessControllerObserverCoverage.java | 233 ++++++++++++++++++ 10 files changed, 456 insertions(+), 22 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControllerObserverCoverage.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlUtil.java index 125a7f5e897c..4d498aca8172 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlUtil.java @@ -856,4 +856,20 @@ public static AccessControlProtos.RevokeRequest buildRevokeRequest(String userna .setUser(ByteString.copyFromUtf8(username)).setPermission(ret)) .build(); } + + public static Permission.Scope toPermissionScope(AccessControlProtos.Permission.Type protoType) { + if (protoType == null) { + return null; + } + switch (protoType) { + case Global: + return Permission.Scope.GLOBAL; + case Namespace: + return Permission.Scope.NAMESPACE; + case Table: + return Permission.Scope.TABLE; + default: + return null; + } + } } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java index ace6f302a198..78a53f725ecf 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.io.VersionedWritable; @@ -62,8 +63,8 @@ public byte code() { } } - @InterfaceAudience.Private - protected enum Scope { + @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC) + public enum Scope { GLOBAL('G'), NAMESPACE('N'), TABLE('T'), diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/ShadedAccessControlUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/ShadedAccessControlUtil.java index cf7c797c98ac..740c80dbceb4 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/ShadedAccessControlUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/ShadedAccessControlUtil.java @@ -328,4 +328,20 @@ public static HasUserPermissionsRequest buildHasUserPermissionsRequest(String us } return builder.build(); } + + public static Permission.Scope toPermissionScope(AccessControlProtos.Permission.Type protoType) { + if (protoType == null) { + return null; + } + switch (protoType) { + case Global: + return Permission.Scope.GLOBAL; + case Namespace: + return Permission.Scope.NAMESPACE; + case Table: + return Permission.Scope.TABLE; + default: + return null; + } + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java index 85ab42b9a555..248446ccfaac 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java @@ -673,7 +673,7 @@ default void preSplitRegionAction(final ObserverContext c, - final RegionInfo regionInfo) { + final RegionInfo regionInfo) throws IOException { } /** @@ -683,7 +683,7 @@ default void preTruncateRegionAction(final ObserverContext c, - RegionInfo regionInfo) { + RegionInfo regionInfo) throws IOException { } /** @@ -693,7 +693,7 @@ default void preTruncateRegion(final ObserverContext c, - RegionInfo regionInfo) { + RegionInfo regionInfo) throws IOException { } /** @@ -703,7 +703,7 @@ default void postTruncateRegion(final ObserverContext c, - final RegionInfo regionInfo) { + final RegionInfo regionInfo) throws IOException { } /** @@ -1837,12 +1837,34 @@ default void postRevoke(ObserverContext ctx, * @param family the table column family, null if don't get table family permission * @param qualifier the table column qualifier, null if don't get table qualifier permission * @throws IOException if something went wrong + * @deprecated Since 2.5.17, 2.6.8, 2.7.0, 3.0.1 and 3.1.0, will be removed in 4.0.0. Use + * {@link #preGetUserPermissions(ObserverContext, String, String, TableName, byte[], byte[], Permission.Scope)} + * instead. */ + @Deprecated default void preGetUserPermissions(ObserverContext ctx, String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier) throws IOException { } + /** + * Called before getting user permissions. + * @param ctx the coprocessor instance's environment + * @param userName the user name, null if get all user permissions + * @param namespace the namespace, null if don't get namespace permission + * @param tableName the table name, null if don't get table permission + * @param family the table column family, null if don't get table family permission + * @param qualifier the table column qualifier, null if don't get table qualifier permission + * @param permissionScope the scope of permission being requested (GLOBAL, NAMESPACE, or TABLE), + * may be null for backward compatibility + * @throws IOException if something went wrong + */ + default void preGetUserPermissions(ObserverContext ctx, + String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier, + Permission.Scope permissionScope) throws IOException { + preGetUserPermissions(ctx, userName, namespace, tableName, family, qualifier); + } + /** * Called after getting user permissions. * @param ctx the coprocessor instance's environment @@ -1852,12 +1874,34 @@ default void preGetUserPermissions(ObserverContext * @param family the table column family, null if don't get table family permission * @param qualifier the table column qualifier, null if don't get table qualifier permission * @throws IOException if something went wrong + * @deprecated Since 2.5.17, 2.6.8, 2.7.0, 3.0.1 and 3.1.0, will be removed in 4.0.0. Use + * {@link #postGetUserPermissions(ObserverContext, String, String, TableName, byte[], byte[], Permission.Scope)} + * instead. */ + @Deprecated default void postGetUserPermissions(ObserverContext ctx, String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier) throws IOException { } + /** + * Called after getting user permissions. + * @param ctx the coprocessor instance's environment + * @param userName the user name, null if get all user permissions + * @param namespace the namespace, null if don't get namespace permission + * @param tableName the table name, null if don't get table permission + * @param family the table column family, null if don't get table family permission + * @param qualifier the table column qualifier, null if don't get table qualifier permission + * @param permissionScope the scope of permission being requested (GLOBAL, NAMESPACE, or TABLE), + * may be null for backward compatibility + * @throws IOException if something went wrong + */ + default void postGetUserPermissions(ObserverContext ctx, + String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier, + Permission.Scope permissionScope) throws IOException { + postGetUserPermissions(ctx, userName, namespace, tableName, family, qualifier); + } + /* * Called before checking if user has permissions. * @param ctx the coprocessor instance's environment diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java index 026eee7c69d1..6a84d3653572 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java @@ -869,7 +869,7 @@ public void call(MasterObserver observer) throws IOException { public void preTruncateRegion(RegionInfo regionInfo) throws IOException { execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { @Override - public void call(MasterObserver observer) { + public void call(MasterObserver observer) throws IOException { observer.preTruncateRegion(this, regionInfo); } }); @@ -882,7 +882,7 @@ public void call(MasterObserver observer) { public void postTruncateRegion(RegionInfo regionInfo) throws IOException { execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { @Override - public void call(MasterObserver observer) { + public void call(MasterObserver observer) throws IOException { observer.postTruncateRegion(this, regionInfo); } }); @@ -2017,22 +2017,46 @@ public void call(MasterObserver observer) throws IOException { }); } + /** + * @deprecated Since 2.5.17, 2.6.8, 2.7.0, 3.0.1 and 3.1.0, will be removed in 4.0.0. Use + * {@link #preGetUserPermissions(String, String, TableName, byte[], byte[], Permission.Scope)} + * instead. + */ + @Deprecated public void preGetUserPermissions(String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier) throws IOException { + preGetUserPermissions(userName, namespace, tableName, family, qualifier, null); + } + + public void preGetUserPermissions(String userName, String namespace, TableName tableName, + byte[] family, byte[] qualifier, Permission.Scope permissionScope) throws IOException { execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { @Override public void call(MasterObserver observer) throws IOException { - observer.preGetUserPermissions(this, userName, namespace, tableName, family, qualifier); + observer.preGetUserPermissions(this, userName, namespace, tableName, family, qualifier, + permissionScope); } }); } + /** + * @deprecated Since 2.5.17, 2.6.8, 2.7.0, 3.0.1 and 3.1.0, will be removed in 4.0.0. Use + * {@link #postGetUserPermissions(String, String, TableName, byte[], byte[], Permission.Scope)} + * instead. + */ + @Deprecated public void postGetUserPermissions(String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier) throws IOException { + postGetUserPermissions(userName, namespace, tableName, family, qualifier, null); + } + + public void postGetUserPermissions(String userName, String namespace, TableName tableName, + byte[] family, byte[] qualifier, Permission.Scope permissionScope) throws IOException { execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { @Override public void call(MasterObserver observer) throws IOException { - observer.postGetUserPermissions(this, userName, namespace, tableName, family, qualifier); + observer.postGetUserPermissions(this, userName, namespace, tableName, family, qualifier, + permissionScope); } }); } 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 c2618b9641ac..9b5be9d4e4b5 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 @@ -2743,6 +2743,7 @@ private void checkMasterProcedureExecutor() throws ServiceException { @Override public MasterProtos.AssignsResponse assigns(RpcController controller, MasterProtos.AssignsRequest request) throws ServiceException { + rpcPreCheck("assigns"); checkMasterProcedureExecutor(); final ProcedureExecutor pe = master.getMasterProcedureExecutor(); final AssignmentManager am = master.getAssignmentManager(); @@ -2770,6 +2771,7 @@ public MasterProtos.AssignsResponse assigns(RpcController controller, @Override public MasterProtos.UnassignsResponse unassigns(RpcController controller, MasterProtos.UnassignsRequest request) throws ServiceException { + rpcPreCheck("unassigns"); checkMasterProcedureExecutor(); final ProcedureExecutor pe = master.getMasterProcedureExecutor(); final AssignmentManager am = master.getAssignmentManager(); @@ -2800,6 +2802,7 @@ public MasterProtos.UnassignsResponse unassigns(RpcController controller, @Override public MasterProtos.BypassProcedureResponse bypassProcedure(RpcController controller, MasterProtos.BypassProcedureRequest request) throws ServiceException { + rpcPreCheck("bypassProcedure"); try { LOG.info("{} bypass procedures={}, waitTime={}, override={}, recursive={}", master.getClientIdAuditPrefix(), request.getProcIdList(), request.getWaitTime(), @@ -2817,6 +2820,7 @@ public MasterProtos.BypassProcedureResponse bypassProcedure(RpcController contro public MasterProtos.ScheduleServerCrashProcedureResponse scheduleServerCrashProcedure( RpcController controller, MasterProtos.ScheduleServerCrashProcedureRequest request) throws ServiceException { + rpcPreCheck("scheduleServerCrashProcedure"); List pids = new ArrayList<>(); for (HBaseProtos.ServerName sn : request.getServerNameList()) { ServerName serverName = ProtobufUtil.toServerName(sn); @@ -2835,7 +2839,7 @@ public MasterProtos.ScheduleServerCrashProcedureResponse scheduleServerCrashProc public MasterProtos.ScheduleSCPsForUnknownServersResponse scheduleSCPsForUnknownServers( RpcController controller, MasterProtos.ScheduleSCPsForUnknownServersRequest request) throws ServiceException { - + rpcPreCheck("scheduleSCPsForUnknownServers"); List pids = new ArrayList<>(); final Set serverNames = master.getAssignmentManager().getRegionStates() .getRegionStates().stream().map(RegionState::getServerName).collect(Collectors.toSet()); @@ -2980,7 +2984,10 @@ public GetUserPermissionsResponse getUserPermissions(RpcController controller, byte[] cq = request.hasColumnQualifier() ? request.getColumnQualifier().toByteArray() : null; Type permissionType = request.hasType() ? request.getType() : null; - master.getMasterCoprocessorHost().preGetUserPermissions(userName, namespace, table, cf, cq); + Permission.Scope permissionScope = + ShadedAccessControlUtil.toPermissionScope(permissionType); + master.getMasterCoprocessorHost().preGetUserPermissions(userName, namespace, table, cf, cq, + permissionScope); List perms = null; if (permissionType == Type.Table) { @@ -3005,8 +3012,8 @@ public GetUserPermissionsResponse getUserPermissions(RpcController controller, } } - master.getMasterCoprocessorHost().postGetUserPermissions(userName, namespace, table, cf, - cq); + master.getMasterCoprocessorHost().postGetUserPermissions(userName, namespace, table, cf, cq, + permissionScope); AccessControlProtos.GetUserPermissionsResponse response = ShadedAccessControlUtil.buildGetUserPermissionsResponse(perms); return response; @@ -3100,6 +3107,7 @@ private boolean shouldSubmitSCP(ServerName serverName) { public HBaseProtos.LogEntry getLogEntries(RpcController controller, HBaseProtos.LogRequest request) throws ServiceException { try { + requirePermission("getLogEntries", Permission.Action.ADMIN); final String logClassName = request.getLogClassName(); Class logClass = Class.forName(logClassName).asSubclass(Message.class); Method method = logClass.getMethod("parseFrom", ByteString.class); @@ -3121,7 +3129,7 @@ public HBaseProtos.LogEntry getLogEntries(RpcController controller, .setLogMessage(balancerRejectionsResponse.toByteString()).build(); } } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException - | InvocationTargetException e) { + | InvocationTargetException | IOException e) { LOG.error("Error while retrieving log entries.", e); throw new ServiceException(e); } 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 70605579bb40..cfc0f113f8df 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 @@ -4122,16 +4122,18 @@ public ClearSlowLogResponses clearSlowLogsResponses(final RpcController controll } @Override + @QosPriority(priority = HConstants.ADMIN_QOS) public HBaseProtos.LogEntry getLogEntries(RpcController controller, HBaseProtos.LogRequest request) throws ServiceException { try { + requirePermission("getLogEntries", Permission.Action.ADMIN); final String logClassName = request.getLogClassName(); Class logClass = Class.forName(logClassName).asSubclass(Message.class); Method method = logClass.getMethod("parseFrom", ByteString.class); if (logClassName.contains("SlowLogResponseRequest")) { SlowLogResponseRequest slowLogResponseRequest = (SlowLogResponseRequest) method.invoke(null, request.getLogMessage()); - final NamedQueueRecorder namedQueueRecorder = this.regionServer.getNamedQueueRecorder(); + final NamedQueueRecorder namedQueueRecorder = regionServer.getNamedQueueRecorder(); final List slowLogPayloads = getSlowLogPayloads(slowLogResponseRequest, namedQueueRecorder); SlowLogResponses slowLogResponses = @@ -4141,7 +4143,7 @@ public HBaseProtos.LogEntry getLogEntries(RpcController controller, .setLogMessage(slowLogResponses.toByteString()).build(); } } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException - | InvocationTargetException e) { + | InvocationTargetException | IOException e) { LOG.error("Error while retrieving log entries.", e); throw new ServiceException(e); } 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 44e18f65654e..a63da714f7e3 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 @@ -874,6 +874,13 @@ public Void run() throws Exception { }); } + @Override + public void preTruncateRegion(final ObserverContext ctx, + final RegionInfo regionInfo) throws IOException { + requirePermission(ctx, "truncateRegion", regionInfo.getTable(), null, null, Action.ADMIN, + Action.CREATE); + } + @Override public TableDescriptor preModifyTable(ObserverContext c, TableName tableName, TableDescriptor currentDesc, TableDescriptor newDesc) throws IOException { @@ -1145,10 +1152,11 @@ public Void run() throws Exception { @Override public void preModifyNamespace(ObserverContext ctx, - NamespaceDescriptor ns) throws IOException { + NamespaceDescriptor currentNsDescriptor, NamespaceDescriptor newNsDescriptor) + throws IOException { // We require only global permission so that // a user with NS admin cannot altering namespace configurations. i.e. namespace quota - requireGlobalPermission(ctx, "modifyNamespace", Action.ADMIN, ns.getName()); + requireGlobalPermission(ctx, "modifyNamespace", Action.ADMIN, newNsDescriptor.getName()); } @Override @@ -1986,7 +1994,10 @@ public void getUserPermissions(RpcController controller, request.hasColumnFamily() ? request.getColumnFamily().toByteArray() : null; final byte[] cq = request.hasColumnQualifier() ? request.getColumnQualifier().toByteArray() : null; - preGetUserPermissions(caller, userName, namespace, table, cf, cq); + AccessControlProtos.Permission.Type protoType = + request.hasType() ? request.getType() : null; + Permission.Scope permissionScope = AccessControlUtil.toPermissionScope(protoType); + preGetUserPermissions(caller, userName, namespace, table, cf, cq, permissionScope); GetUserPermissionsRequest getUserPermissionsRequest = null; if (request.getType() == AccessControlProtos.Permission.Type.Table) { getUserPermissionsRequest = GetUserPermissionsRequest.newBuilder(table).withFamily(cf) @@ -2408,9 +2419,26 @@ private void preGrantOrRevoke(User caller, String request, UserPermission userPe @Override public void preGetUserPermissions(ObserverContext ctx, - String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier) + String userName, String namespace, TableName tableName, byte[] family, byte[] qualifier, + Permission.Scope permissionScope) throws IOException { + preGetUserPermissions(getActiveUser(ctx), userName, namespace, tableName, family, qualifier, + permissionScope); + } + + private void preGetUserPermissions(User caller, String userName, String namespace, + TableName tableName, byte[] family, byte[] qualifier, Permission.Scope permissionScope) throws IOException { - preGetUserPermissions(getActiveUser(ctx), userName, namespace, tableName, family, qualifier); + if (permissionScope == Permission.Scope.TABLE) { + accessChecker.requirePermission(caller, "getUserPermissions", tableName, family, qualifier, + userName, Action.ADMIN); + } else if (permissionScope == Permission.Scope.NAMESPACE) { + accessChecker.requireNamespacePermission(caller, "getUserPermissions", namespace, userName, + Action.ADMIN); + } else if (permissionScope == Permission.Scope.GLOBAL) { + accessChecker.requirePermission(caller, "getUserPermissions", userName, Action.ADMIN); + } else { + preGetUserPermissions(caller, userName, namespace, tableName, family, qualifier); + } } private void preGetUserPermissions(User caller, String userName, String namespace, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java index f471b64bffa0..d784f8d486a2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java @@ -3405,6 +3405,68 @@ public void testGetUserPermissions() throws Throwable { } } + @Test + public void testGetUserPermissionsScopeBasedAuthorization() throws Throwable { + // Regression test: authorization must be based on permissionScope (derived from + // the type field), not on the tableName field. A user with only table-level ADMIN + // should not be able to request global permissions by passing tableName with + // permissionScope=GLOBAL. + AccessTestAction tablePermWithGlobalScope = new AccessTestAction() { + @Override + public Object run() throws Exception { + ACCESS_CONTROLLER.preGetUserPermissions(ObserverContextImpl.createAndPrepare(CP_ENV), null, + null, TEST_TABLE, null, null, Permission.Scope.GLOBAL); + return null; + } + }; + + // USER_ADMIN has global ADMIN, should be allowed + verifyAllowed(tablePermWithGlobalScope, SUPERUSER, USER_ADMIN); + // Users with only table-level ADMIN should be denied + verifyDenied(tablePermWithGlobalScope, USER_OWNER, USER_ADMIN_CF, USER_CREATE, USER_RW, USER_RO, + USER_NONE); + + AccessTestAction tablePermWithNamespaceScope = new AccessTestAction() { + @Override + public Object run() throws Exception { + ACCESS_CONTROLLER.preGetUserPermissions(ObserverContextImpl.createAndPrepare(CP_ENV), null, + TEST_TABLE.getNamespaceAsString(), TEST_TABLE, null, null, Permission.Scope.NAMESPACE); + return null; + } + }; + + // Users without namespace ADMIN should be denied + verifyDenied(tablePermWithNamespaceScope, USER_OWNER, USER_ADMIN_CF, USER_CREATE, USER_RW, + USER_RO, USER_NONE); + + AccessTestAction tablePermWithTableScope = new AccessTestAction() { + @Override + public Object run() throws Exception { + ACCESS_CONTROLLER.preGetUserPermissions(ObserverContextImpl.createAndPrepare(CP_ENV), null, + null, TEST_TABLE, null, null, Permission.Scope.TABLE); + return null; + } + }; + + // Users with table ADMIN should be allowed + verifyAllowed(tablePermWithTableScope, SUPERUSER, USER_ADMIN, USER_OWNER); + verifyDenied(tablePermWithTableScope, USER_ADMIN_CF, USER_CREATE, USER_RW, USER_RO, USER_NONE); + + AccessTestAction globalPermWithNullScope = new AccessTestAction() { + @Override + public Object run() throws Exception { + ACCESS_CONTROLLER.preGetUserPermissions(ObserverContextImpl.createAndPrepare(CP_ENV), null, + null, null, null, null, null); + return null; + } + }; + + // null scope falls back to old behavior (no tableName -> require global ADMIN) + verifyAllowed(globalPermWithNullScope, SUPERUSER, USER_ADMIN); + verifyDenied(globalPermWithNullScope, USER_OWNER, USER_ADMIN_CF, USER_CREATE, USER_RW, USER_RO, + USER_NONE); + } + @Test public void testHasPermission() throws Throwable { Connection conn = null; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControllerObserverCoverage.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControllerObserverCoverage.java new file mode 100644 index 000000000000..4dfd24d6eee8 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControllerObserverCoverage.java @@ -0,0 +1,233 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import static org.junit.jupiter.api.Assertions.fail; + +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +import org.apache.hadoop.hbase.coprocessor.BulkLoadObserver; +import org.apache.hadoop.hbase.coprocessor.EndpointObserver; +import org.apache.hadoop.hbase.coprocessor.MasterObserver; +import org.apache.hadoop.hbase.coprocessor.RegionObserver; +import org.apache.hadoop.hbase.coprocessor.RegionServerObserver; +import org.apache.hadoop.hbase.testclassification.SecurityTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Verifies that AccessController implements every security-relevant method declared in the five + * observer interfaces it claims to implement: MasterObserver, RegionObserver, RegionServerObserver, + * EndpointObserver, BulkLoadObserver. + *

+ * If a new hook is added to any of these interfaces and AccessController does not override it, the + * default no-op implementation will silently skip the permission check — a potential privilege + * escalation. This test catches that at build time. + *

+ * Skipped methods are determined by two mechanisms: + *

    + *
  • Pattern rules — methods matching these patterns are always safe to skip: + *
      + *
    • {@code post*} — post-operation notifications; the operation has already been authorized and + * executed.
      • + *
    • {@code pre*Action} — procedure-level action hooks; authorization happens at the RPC layer in + * the corresponding {@code pre*} hook.
      • + *
    + *
    • + *
  • Explicit whitelist — methods that don't match the above rules but are still safe to + * skip (internal lifecycle hooks, deprecated overloads with default delegation, read-only queries). + * Each entry has a justification comment.
    • + *
+ */ +@Tag(SecurityTests.TAG) +@Tag(SmallTests.TAG) +public class TestAccessControllerObserverCoverage { + + /** + * Explicit whitelist for methods that don't match the pattern rules but are intentionally not + * overridden in AccessController. + *

+ * Use simple method name (covers all overloads) or full signature key + * "methodName(ParamType1,ParamType2,..." using simple class names. + */ + private static final Set WHITELIST = new HashSet<>(Arrays.asList( + + // --- Internal lifecycle hooks (not client-facing RPCs) --- + // Store file / WAL internal hooks + "preStoreFileReaderOpen", "preStoreScannerOpen", "preCommitStoreFile", "preReplayWALs", + "preWALRestore", + // Master internal + "preMasterStoreFlush", + // Lifecycle markers (not triggered by client RPC) + "preMasterInitialization", "preCreateTableRegionsInfos", + // --- Read-only query hooks (no mutation, no authorization needed) --- + "preGetClusterMetrics", "preGetTableNames", "preListNamespaceDescriptors", "preListNamespaces", + // RSGroup related methods, access control is implemented in RSGroupAdminEndpoint + "preMoveServers", "preMoveServersAndTables", "preMoveTables", "preRemoveServers", + + // --- Deprecated overloads: interface default delegates to non-deprecated --- + // prePut(3-arg) delegates to prePut(4-arg Durability) + "prePut(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Put,org.apache.hadoop.hbase.wal.WALEdit)", + // preDelete(3-arg) delegates to preDelete(4-arg Durability) + "preDelete(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Delete,org.apache.hadoop.hbase.wal.WALEdit)", + // preAppend(3-arg) delegates to preAppend(2-arg deprecated) + "preAppend(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Append,org.apache.hadoop.hbase.wal.WALEdit)", + // preAppendAfterRowLock(2-arg) delegates to preAppendAfterRowLock(1-arg deprecated) + "preAppendAfterRowLock(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Append)", + // preIncrement(3-arg) delegates to preIncrement(2-arg deprecated) + "preIncrement(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Increment,org.apache.hadoop.hbase.wal.WALEdit)", + // preIncrementAfterRowLock(2-arg) delegates to preIncrementAfterRowLock(1-arg deprecated) + "preIncrementAfterRowLock(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.Increment)", + // preCheckAndMutate delegates to preCheckAndPut/preCheckAndDelete + "preCheckAndMutate(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.CheckAndMutate,org.apache.hadoop.hbase.client.CheckAndMutateResult)", + // preCheckAndMutateAfterRowLock delegates to + // preCheckAndPutAfterRowLock/preCheckAndDeleteAfterRowLock + "preCheckAndMutateAfterRowLock(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.CheckAndMutate,org.apache.hadoop.hbase.client.CheckAndMutateResult)", + // Filter-based CheckAnd* are deprecated; framework uses byte[] overloads + // Note: Class.getName() returns "[B" for byte[], not "byte[]" + "preCheckAndPut(org.apache.hadoop.hbase.coprocessor.ObserverContext,[B,org.apache.hadoop.hbase.filter.Filter,org.apache.hadoop.hbase.client.Put,boolean)", + "preCheckAndPutAfterRowLock(org.apache.hadoop.hbase.coprocessor.ObserverContext,[B,org.apache.hadoop.hbase.filter.Filter,org.apache.hadoop.hbase.client.Put,boolean)", + "preCheckAndDelete(org.apache.hadoop.hbase.coprocessor.ObserverContext,[B,org.apache.hadoop.hbase.filter.Filter,org.apache.hadoop.hbase.client.Delete,boolean)", + "preCheckAndDeleteAfterRowLock(org.apache.hadoop.hbase.coprocessor.ObserverContext,[B,org.apache.hadoop.hbase.filter.Filter,org.apache.hadoop.hbase.client.Delete,boolean)", + // Deprecated preGetUserPermissions(6-arg) delegates to preGetUserPermissions(7-arg with Scope) + "preGetUserPermissions(ObserverContext,String,String,TableName,byte[],byte[])", + // Deprecated WAL-append / timestamp hooks + "prePrepareTimeStampForDeleteVersion", "preWALAppend", + // Deprecated preModifyXXX where we do not pass the old descriptor + "preModifyNamespace(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.NamespaceDescriptor)", + "preModifyTable(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.TableName,org.apache.hadoop.hbase.client.TableDescriptor)", + // Deprecated dangerous force unassign + "preUnassign(org.apache.hadoop.hbase.coprocessor.ObserverContext,org.apache.hadoop.hbase.client.RegionInfo,boolean)", + // --- Replication sink is a trusted internal cluster-to-cluster operation --- + "preReplicationSinkBatchMutate")); + + private static final Class[] OBSERVER_INTERFACES = + { MasterObserver.class, RegionObserver.class, RegionServerObserver.class, + EndpointObserver.class, BulkLoadObserver.class }; + + /** + * Returns true if the method matches a pattern rule that makes it safe to skip without an + * explicit whitelist entry. + */ + private static boolean matchesSkipPattern(Class iface, Method m) { + String name = m.getName(); + // All post* methods are post-operation notifications. + // Permission checks must happen before the operation, not after. + if (name.startsWith("post")) { + return true; + } + // *Action suffix on pre* hooks are procedure-level callbacks. + // Authorization is done at the RPC layer in the corresponding pre* hook. + if (name.endsWith("Action")) { + return true; + } + // RSGroup related access control is implemented separated in RSGroupAdminEndpoint + if (name.contains("RSGroup")) { + return true; + } + // RegionObserver internal storage hooks: flush, compaction, in-memory + // compaction. These are sub-step callbacks within a region storage + // operation. The region-level entry hook (preFlush, preCompact) already + // handles authorization in AccessController. + if ( + iface == RegionObserver.class && (name.startsWith("preFlush") || name.startsWith("preCompact") + || name.startsWith("preMemStore")) + ) { + return true; + } + return false; + } + + private static String methodSignatureKey(Method m) { + String paramTypes = Arrays.stream(m.getParameterTypes()).map(Class::getSimpleName) + .collect(Collectors.joining(",")); + return m.getName() + "(" + paramTypes + ")"; + } + + private static String fullMethodSignatureKey(Method m) { + String paramTypes = + Arrays.stream(m.getParameterTypes()).map(Class::getName).collect(Collectors.joining(",")); + return m.getName() + "(" + paramTypes + ")"; + } + + private static boolean isMethodImplemented(Class implClass, Method ifaceMethod) { + Class clazz = implClass; + while (clazz != null) { + for (Method m : clazz.getDeclaredMethods()) { + if ( + m.getName().equals(ifaceMethod.getName()) + && Arrays.equals(m.getParameterTypes(), ifaceMethod.getParameterTypes()) + ) { + return true; + } + } + clazz = clazz.getSuperclass(); + } + return false; + } + + @Test + public void testAllObserverMethodsAreImplemented() { + Set missing = new TreeSet<>(); + + for (Class iface : OBSERVER_INTERFACES) { + for (Method m : iface.getMethods()) { + if (m.getDeclaringClass() == Object.class) { + continue; + } + if (!m.getDeclaringClass().equals(iface)) { + continue; + } + + if (matchesSkipPattern(iface, m)) { + continue; + } + + String simpleName = m.getName(); + String simpleKey = methodSignatureKey(m); + String fullKey = fullMethodSignatureKey(m); + + if ( + WHITELIST.contains(simpleName) || WHITELIST.contains(simpleKey) + || WHITELIST.contains(fullKey) + ) { + continue; + } + + if (!isMethodImplemented(AccessController.class, m)) { + missing.add(" " + iface.getSimpleName() + "." + simpleKey); + } + } + } + + if (!missing.isEmpty()) { + StringBuilder sb = new StringBuilder(); + sb.append("AccessController does not implement the following observer methods.\n"); + sb.append("Either override them in AccessController (with permission checks),\n"); + sb.append("or add them to the WHITELIST with a justification comment.\n\n"); + sb.append("Missing methods:\n"); + missing.forEach(m -> sb.append(m).append("\n")); + fail(sb.toString()); + } + } +}