[HBASE-27528] log duplication issues in MasterRpcServices

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 2.6.0, 3.0.0-alpha-4
• Component/s: logging, master, rpc, security
• Hadoop Flags: Reviewed
• Assignee: Beibei Zhao
• Reporter: Beibei Zhao
• Created: 10/Dec/22 10:23
• Updated: 17/Mar/23 07:04
• Resolved: 20/Jan/23 08:59

Description

MasterRpcServices record audit log in privileged operations (grant, revoke) and vital apis like "execMasterService".

  public RevokeResponse revoke(RpcController controller, RevokeRequest request)
    throws ServiceException {
    try {
      ......
        server.cpHost.preRevoke(userPermission); // has audit log in AccessChecker
       ...... // removeUserPermission
        User caller = RpcServer.getRequestUser().orElse(null);
        if (AUDITLOG.isTraceEnabled()) {
          // audit log should record all permission changes
          String remoteAddress = RpcServer.getRemoteAddress().map(InetAddress::toString).orElse("");
          AUDITLOG.trace("User {} (remote address: {}) revoked permission {}", caller,
            remoteAddress, userPermission);
        }
        ......
  }

but I found a path from server.cpHost.preRevoke(userPermission); to AccessChecker audit log, which caused log duplication.
grant/revoke -> AccessController.preGrant/Revoke -> preGrantOrRevoke -> AccessChecker.requireGlobalPermission/... -> logResult

public void requireGlobalPermission(User user, String request, Action perm, String namespace)
    throws IOException {
    AuthResult authResult;
    if (authManager.authorizeUserGlobal(user, perm)) {
      authResult = AuthResult.allow(request, "Global check allowed", user, perm, null);
      authResult.getParams().setNamespace(namespace);
      logResult(authResult);
    } else {
      authResult = AuthResult.deny(request, "Global check failed", user, perm, null);
      authResult.getParams().setNamespace(namespace);
      logResult(authResult);
      throw new AccessDeniedException(
        "Insufficient permissions for user '" + (user != null ? user.getShortName() : "null")
          + "' (global, action=" + perm.toString() + ")");
    }
  }

  public static void logResult(AuthResult result) {
    if (AUDITLOG.isTraceEnabled()) {
      User user = result.getUser();
      UserGroupInformation ugi = user != null ? user.getUGI() : null;
      AUDITLOG.trace(
        "Access {} for user {}; reason: {}; remote address: {}; request: {}; context: {};"
          + "auth method: {}",
        (result.isAllowed() ? "allowed" : "denied"),
        (user != null ? user.getShortName() : "UNKNOWN"), result.getReason(),
        RpcServer.getRemoteAddress().map(InetAddress::toString).orElse(""), result.getRequest(),
        result.toContextString(), ugi != null ? ugi.getAuthenticationMethod() : "UNKNOWN");
    }
  }

Since AccessChecker integrates auditlogs for permission check, I'll delete the log in MasterRpcServices.

There must be more problems like this, I' ll check it later and commit the code.

There are many "write" operations like "deleteTable", which may cause security problems, should also record an audit log.

Issue Links

• links to: GitHub Pull Request #4951