Add errorprone to prevent Log4j direct usage (#3759)

* Replace Log4j2 API with SLF4j API

* Replace explicit Log4J2 with util call

* Replace ThreadContext with Slf4J's MDC in test

* Inspect raw request parameter for admin_changeLogLevel

* Add errorprone rule to prevent the creation Log4j2 loggers

Signed-off-by: Diego López León <dieguitoll@gmail.com>
Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/AcceptanceTestBase.java

@@ -57,7 +57,6 @@ import java.math.BigInteger;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
-import org.apache.logging.log4j.ThreadContext;
 import org.junit.After;
 import org.junit.Rule;
 import org.junit.rules.TestName;
@@ -65,6 +64,7 @@ import org.junit.rules.TestWatcher;
 import org.junit.runner.Description;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.slf4j.MDC;
 
 public class AcceptanceTestBase {
 
@@ -184,8 +184,8 @@ public class AcceptanceTestBase {
 
         @Override
         protected void starting(final Description description) {
-          ThreadContext.put("test", description.getMethodName());
-          ThreadContext.put("class", description.getClassName());
+          MDC.put("test", description.getMethodName());
+          MDC.put("class", description.getClassName());
 
           final String errorMessage = "Uncaught exception in thread \"{}\"";
           Thread.currentThread()

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/privacy/PrivacyGroupAcceptanceTest.java

@@ -20,6 +20,7 @@ import static org.web3j.utils.Restriction.RESTRICTED;
 import org.hyperledger.besu.tests.acceptance.dsl.privacy.PrivacyAcceptanceTestBase;
 import org.hyperledger.besu.tests.acceptance.dsl.privacy.PrivacyNode;
 import org.hyperledger.besu.tests.web3j.generated.EventEmitter;
+import org.hyperledger.besu.util.Log4j2ConfiguratorUtil;
 import org.hyperledger.enclave.testutil.EnclaveType;
 
 import java.io.IOException;
@@ -28,10 +29,6 @@ import java.util.Collection;
 import java.util.Optional;
 
 import org.apache.logging.log4j.Level;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.core.LoggerContext;
-import org.apache.logging.log4j.core.config.Configuration;
-import org.apache.logging.log4j.core.config.LoggerConfig;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -90,12 +87,7 @@ public class PrivacyGroupAcceptanceTest extends PrivacyAcceptanceTestBase {
 
   @Test
   public void nodeCanCreatePrivacyGroup() {
-
-    final LoggerContext ctx = (LoggerContext) LogManager.getContext(false);
-    final Configuration config = ctx.getConfiguration();
-    final LoggerConfig loggerConfig = config.getLoggerConfig(LogManager.ROOT_LOGGER_NAME);
-    loggerConfig.setLevel(Level.DEBUG);
-    ctx.updateLoggers();
+    Log4j2ConfiguratorUtil.setLevel("", Level.DEBUG);
     final String privacyGroupId =
         alice.execute(
             privacyTransactions.createPrivacyGroup(

besu/src/main/java/org/hyperledger/besu/Besu.java

@@ -68,13 +68,13 @@ public final class Besu {
               + e.getMessage());
     }
     final Logger logger = LoggerFactory.getLogger(Besu.class);
-    Thread.setDefaultUncaughtExceptionHandler(log4jExceptionHandler(logger));
-    Thread.currentThread().setUncaughtExceptionHandler(log4jExceptionHandler(logger));
+    Thread.setDefaultUncaughtExceptionHandler(slf4jExceptionHandler(logger));
+    Thread.currentThread().setUncaughtExceptionHandler(slf4jExceptionHandler(logger));
 
     return logger;
   }
 
-  private static Thread.UncaughtExceptionHandler log4jExceptionHandler(final Logger logger) {
+  private static Thread.UncaughtExceptionHandler slf4jExceptionHandler(final Logger logger) {
     return (thread, error) -> {
       if (logger.isErrorEnabled()) {
         logger.error(String.format("Uncaught exception in thread \"%s\"", thread.getName()), error);

besu/src/main/java/org/hyperledger/besu/cli/options/stable/LoggingLevelOption.java

@@ -14,8 +14,12 @@
  */
 package org.hyperledger.besu.cli.options.stable;
 
+import java.util.Set;
+
 import org.apache.logging.log4j.Level;
 import picocli.CommandLine;
+import picocli.CommandLine.Model.CommandSpec;
+import picocli.CommandLine.Spec;
 
 public class LoggingLevelOption {
 
@@ -23,18 +27,24 @@ public class LoggingLevelOption {
     return new LoggingLevelOption();
   }
 
+  private static final Set<String> ACCEPTED_VALUES =
+      Set.of("OFF", "ERROR", "WARN", "INFO", "DEBUG", "TRACE", "ALL");
+  @Spec CommandSpec spec;
   private Level logLevel;
 
   @CommandLine.Option(
       names = {"--logging", "-l"},
       paramLabel = "<LOG VERBOSITY LEVEL>",
       description = "Logging verbosity levels: OFF, ERROR, WARN, INFO, DEBUG, TRACE, ALL")
-  public void setLogLevel(final Level logLevel) {
-    if (Level.FATAL.equals(logLevel)) {
+  public void setLogLevel(final String logLevel) {
+    if ("FATAL".equalsIgnoreCase(logLevel)) {
       System.out.println("FATAL level is deprecated");
       this.logLevel = Level.ERROR;
+    } else if (ACCEPTED_VALUES.contains(logLevel.toUpperCase())) {
+      this.logLevel = Level.getLevel(logLevel.toUpperCase());
     } else {
-      this.logLevel = logLevel;
+      throw new CommandLine.ParameterException(
+          spec.commandLine(), "Unknown logging value: " + logLevel);
     }
   }
 

besu/src/main/java/org/hyperledger/besu/controller/MergeBesuControllerBuilder.java

@@ -41,12 +41,12 @@ import java.util.Optional;
 import java.util.OptionalLong;
 import java.util.concurrent.atomic.AtomicReference;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class MergeBesuControllerBuilder extends BesuControllerBuilder {
   private final AtomicReference<SyncState> syncState = new AtomicReference<>();
-  private static final Logger LOG = LogManager.getLogger(MergeBesuControllerBuilder.class);
+  private static final Logger LOG = LoggerFactory.getLogger(MergeBesuControllerBuilder.class);
 
   @Override
   protected MiningCoordinator createMiningCoordinator(

besu/src/test/java/org/hyperledger/besu/cli/options/stable/LoggingLevelOptionTest.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright contributors to Hyperledger Besu.
+ *
+ * Licensed 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.cli.options.stable;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Answers.RETURNS_DEEP_STUBS;
+
+import java.util.Arrays;
+
+import org.apache.logging.log4j.Level;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+import picocli.CommandLine.Model.CommandSpec;
+import picocli.CommandLine.ParameterException;
+
+public class LoggingLevelOptionTest {
+
+  private LoggingLevelOption levelOption;
+
+  @Before
+  public void setUp() {
+    levelOption = new LoggingLevelOption();
+  }
+
+  @Test
+  public void fatalLevelEqualsToError() {
+    levelOption.setLogLevel("fatal");
+    assertThat(levelOption.getLogLevel()).isEqualTo(Level.ERROR);
+  }
+
+  @Test
+  public void setsExpectedLevels() {
+    Arrays.stream(Level.values())
+        .filter(level -> !Level.FATAL.equals(level))
+        .forEach(
+            level -> {
+              levelOption.setLogLevel(level.name());
+              assertThat(levelOption.getLogLevel()).isEqualTo(level);
+            });
+  }
+
+  @Test(expected = ParameterException.class)
+  public void failsOnUnknownLevel() {
+    levelOption.spec = Mockito.mock(CommandSpec.class, RETURNS_DEEP_STUBS);
+    levelOption.setLogLevel("unknown");
+  }
+}

errorprone-checks/src/main/java/org/hyperledger/errorpronechecks/BannedMethod.java

@@ -44,7 +44,9 @@ public class BannedMethod extends BugChecker implements MethodInvocationTreeMatc
           staticMethod().onClass("com.google.common.base.Objects").withAnyName(),
           "Do not use com.google.common.base.Objects methods, use java.util.Objects methods instead.",
           staticMethod().onClass("org.junit.Assert"),
-          "Do not use junit assertions. Use assertj assertions instead.");
+          "Do not use junit assertions. Use assertj assertions instead.",
+          staticMethod().onClass("org.apache.logging.log4j.LogManager"),
+          "Do not use org.apache.logging.log4j.LogManager, use org.slf4j.LoggerFactory methods instead.");
 
   @Override
   public Description matchMethodInvocation(

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AdminChangeLogLevel.java

@@ -25,6 +25,7 @@ import org.hyperledger.besu.util.Log4j2ConfiguratorUtil;
 
 import java.util.Arrays;
 import java.util.Optional;
+import java.util.Set;
 
 import org.apache.logging.log4j.Level;
 import org.slf4j.Logger;
@@ -33,6 +34,8 @@ import org.slf4j.LoggerFactory;
 public class AdminChangeLogLevel implements JsonRpcMethod {
 
   private static final Logger LOG = LoggerFactory.getLogger(AdminChangeLogLevel.class);
+  private static final Set<String> VALID_PARAMS =
+      Set.of("OFF", "ERROR", "WARN", "INFO", "DEBUG", "TRACE", "ALL");
 
   @Override
   public String getName() {
@@ -42,7 +45,12 @@ public class AdminChangeLogLevel implements JsonRpcMethod {
   @Override
   public JsonRpcResponse response(final JsonRpcRequestContext requestContext) {
     try {
-      final Level logLevel = requestContext.getRequiredParameter(0, Level.class);
+      final String rawLogLevel = requestContext.getRequiredParameter(0, String.class);
+      if (!VALID_PARAMS.contains(rawLogLevel)) {
+        return new JsonRpcErrorResponse(
+            requestContext.getRequest().getId(), JsonRpcError.INVALID_PARAMS);
+      }
+      final Level logLevel = Level.toLevel(rawLogLevel);
       final Optional<String[]> optionalLogFilters =
           requestContext.getOptionalParameter(1, String[].class);
       optionalLogFilters.ifPresentOrElse(

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AdminChangeLogLevelTest.java

@@ -22,9 +22,9 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
+import org.hyperledger.besu.util.Log4j2ConfiguratorUtil;
 
 import org.apache.logging.log4j.Level;
-import org.apache.logging.log4j.core.config.Configurator;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -40,7 +40,7 @@ public class AdminChangeLogLevelTest {
   @Before
   public void before() {
     adminChangeLogLevel = new AdminChangeLogLevel();
-    Configurator.setAllLevels("", Level.INFO);
+    Log4j2ConfiguratorUtil.setAllLevels("", Level.INFO);
   }
 
   @Test
@@ -52,7 +52,7 @@ public class AdminChangeLogLevelTest {
   public void shouldReturnCorrectResponseWhenRequestHasLogLevel() {
     final JsonRpcRequestContext request =
         new JsonRpcRequestContext(
-            new JsonRpcRequest("2.0", "admin_changeLogLevel", new Object[] {Level.DEBUG}));
+            new JsonRpcRequest("2.0", "admin_changeLogLevel", new Object[] {Level.DEBUG.name()}));
     final JsonRpcResponse expectedResponse =
         new JsonRpcSuccessResponse(request.getRequest().getId());
 
@@ -71,7 +71,9 @@ public class AdminChangeLogLevelTest {
     final JsonRpcRequestContext request =
         new JsonRpcRequestContext(
             new JsonRpcRequest(
-                "2.0", "admin_changeLogLevel", new Object[] {Level.DEBUG, new String[] {"com"}}));
+                "2.0",
+                "admin_changeLogLevel",
+                new Object[] {Level.DEBUG.name(), new String[] {"com"}}));
     final JsonRpcResponse expectedResponse =
         new JsonRpcSuccessResponse(request.getRequest().getId());
 

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/AbstractRetryingTest.java

@@ -14,8 +14,8 @@
  */
 package org.hyperledger.besu.ethereum.vm;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.core.LoggerContext;
+import org.hyperledger.besu.util.Log4j2ConfiguratorUtil;
+
 import org.junit.Before;
 import org.junit.Test;
 
@@ -73,7 +73,7 @@ public abstract class AbstractRetryingTest {
   }
 
   private void resetLogging() {
-    ((LoggerContext) LogManager.getContext(false)).reconfigure();
+    Log4j2ConfiguratorUtil.reconfigure();
   }
 
   /** Subclasses should implement this method to run the actual JUnit test. */