Skip to content

Commit

Permalink
Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816)
Browse files Browse the repository at this point in the history
### Motivation
Upgrades to log4j2 to get rid of CVE-2019-17571.  

### Changes

The migration of log4j has been done mainly taking the official guidelines: https://logging.apache.org/log4j/2.x/manual/migration.html.
In this PR, the following changes are included:
- Replacement of `slf4j-log4j12` by `log4j-1.2-api`. Also included the `log4j-slf4j-impl` binding as well as the `log4j-core` library.
- Changes in `pom`, `gradle` and license files to reflect the above library upgrade.
- Test classes `TestOrderedExecutorDecorators`, `LoggerOutput`, `MdcContextTest`, as well as the class `FIleSystemUpgrade` made use of log4j1.2 API. This PR attempts to keep the same functionality with the new APIs.

### Verification
- Existing tests are passing.
- log4j1.2 is removed from project: apache#2816 (comment)
- Using `localbookie`, we observe that logs are shown correctly:
```
2021-10-07T16:04:23,757 - INFO  - [main:GarbageCollectorThread@245] - Minor Compaction : enabled=true, threshold=0.20000000298023224, interval=3600000
2021-10-07T16:04:23,760 - INFO  - [main:GarbageCollectorThread@247] - Major Compaction : enabled=true, threshold=0.800000011920929, interval=86400000
2021-10-07T16:04:23,952 - INFO  - [main:BookieImpl@920] - Finished replaying journal in 2 ms.
2021-10-07T16:04:23,958 - INFO  - [SyncThread-7-1:SyncThread@135] - Flush ledger storage at checkpoint CheckpointList{checkpoints=[LogMark: logFileId - 0 , logFileOffset - 0]}.
2021-10-07T16:04:23,980 - INFO  - [main:BookieImpl@1010] - Finished reading journal, starting bookie
2021-10-07T16:04:24,011 - INFO  - [BookieJournal-5000:Journal@919] - Starting journal on /tmp/localbookkeeper06554024139823286046test/current
2021-10-07T16:04:24,031 - INFO  - [ForceWriteThread:Journal$ForceWriteThread@478] - ForceWrite Thread started
2021-10-07T16:04:24,048 - INFO  - [BookieJournal-5000:JournalChannel@169] - Opening journal /tmp/localbookkeeper06554024139823286046test/current/17c5b11c65b.txn
```
In addition to that, if we change the `log4j.properties` file, the changes are reflected in the console output, meaning that the legacy configuration works and changes can be correctly applied:
```
Over Replicated Ledger Deletion : enabled=true, interval=86400000
Minor Compaction : enabled=true, threshold=0.20000000298023224, interval=3600000
Major Compaction : enabled=true, threshold=0.800000011920929, interval=86400000
Finished replaying journal in 5 ms.
Flush ledger storage at checkpoint CheckpointList{checkpoints=[LogMark: logFileId - 0 , logFileOffset - 0]}.
Finished reading journal, starting bookie
Starting journal on /tmp/localbookkeeper015049859959001160726test/current
ForceWrite Thread started
Opening journal /tmp/localbookkeeper015049859959001160726test/current/17c5b143063.txn
```
More verifications that logging works properly related to other Bookkeeper sub-components impacted may be needed.

Master Issue: apache#2815
  • Loading branch information
RaulGracia authored Oct 15, 2021
1 parent 0465052 commit a522fa3
Show file tree
Hide file tree
Showing 28 changed files with 210 additions and 98 deletions.
5 changes: 3 additions & 2 deletions bookkeeper-common/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ dependencies {
testImplementation depLibs.commonsLang3
testImplementation depLibs.hamcrest
testImplementation depLibs.junit
testImplementation depLibs.log4j
testImplementation depLibs.log4jSlf4jImpl
testImplementation depLibs.log4j12api
testImplementation depLibs.log4jCore
testImplementation depLibs.mockito
testImplementation depLibs.slf4jLog4j

annotationProcessor depLibs.lombok
testAnnotationProcessor depLibs.lombok
Expand Down
20 changes: 20 additions & 0 deletions bookkeeper-common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,26 @@
<artifactId>commons-lang3</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>${slf4j.version}</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
<version>${log4j.version}</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<version>${log4j.version}</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<version>${log4j.version}</version>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ protected OrderedExecutor(String baseName, int numThreads, ThreadFactory threadF
try {
CpuAffinity.acquireCore();
} catch (Throwable t) {
log.warn("Failed to acquire CPU core for thread {}", Thread.currentThread().getName(),
log.warn("Failed to acquire CPU core for thread {}: {}", Thread.currentThread().getName(),
t.getMessage(), t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,18 @@
import static org.mockito.AdditionalAnswers.answerVoid;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.spy;

import java.util.Queue;
import java.util.UUID;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.TimeUnit;

import org.apache.log4j.Appender;
import org.apache.log4j.Level;
import org.apache.log4j.LogManager;
import org.apache.log4j.MDC;
import org.apache.log4j.spi.LoggingEvent;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.NullAppender;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -53,7 +53,7 @@ public class TestOrderedExecutorDecorators {
private static final Logger log = LoggerFactory.getLogger(TestOrderedExecutorDecorators.class);
private static final String MDC_KEY = "mdc-key";

private Appender mockAppender;
private NullAppender mockAppender;
private final Queue<String> capturedEvents = new ConcurrentLinkedQueue<>();

public static String mdcFormat(Object mdc, String message) {
Expand All @@ -63,21 +63,25 @@ public static String mdcFormat(Object mdc, String message) {
@Before
public void setUp() throws Exception {
MDC.clear();
mockAppender = mock(Appender.class);
when(mockAppender.getName()).thenReturn("MockAppender");

LogManager.getRootLogger().addAppender(mockAppender);
LogManager.getRootLogger().setLevel(Level.INFO);

doAnswer(answerVoid((LoggingEvent event) -> {
capturedEvents.add(mdcFormat(event.getMDC(MDC_KEY),
event.getRenderedMessage()));
})).when(mockAppender).doAppend(any());
LoggerContext lc = (LoggerContext) org.apache.logging.log4j.LogManager.getContext(false);
mockAppender = spy(NullAppender.createAppender(UUID.randomUUID().toString()));
mockAppender.start();
lc.getConfiguration().addAppender(mockAppender);
lc.getRootLogger().addAppender(lc.getConfiguration().getAppender(mockAppender.getName()));
lc.getConfiguration().getRootLogger().setLevel(Level.INFO);
lc.updateLoggers();

doAnswer(answerVoid((LogEvent event) -> {
capturedEvents.add(mdcFormat(event.getContextData().getValue(MDC_KEY),
event.getMessage().getFormattedMessage()));
})).when(mockAppender).append(any());
}

@After
public void tearDown() throws Exception {
LogManager.getRootLogger().removeAppender(mockAppender);
LoggerContext lc = (LoggerContext) org.apache.logging.log4j.LogManager.getContext(false);
lc.getRootLogger().removeAppender(lc.getConfiguration().getAppender(mockAppender.getName()));
lc.updateLoggers();
capturedEvents.clear();
MDC.clear();
}
Expand Down
2 changes: 1 addition & 1 deletion bookkeeper-dist/all/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ dependencies {
compileOnly depLibs.lombok
compileOnly depLibs.spotbugsAnnotations

implementation depLibs.slf4jLog4j
implementation depLibs.log4j12api


testCompileOnly depLibs.lombok
Expand Down
12 changes: 10 additions & 2 deletions bookkeeper-dist/all/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,16 @@

<!-- slf4j binding -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>

<dependency>
Expand Down
12 changes: 10 additions & 2 deletions bookkeeper-dist/bkctl/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,16 @@

<!-- slf4j binding -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>
</dependencies>

Expand Down
2 changes: 1 addition & 1 deletion bookkeeper-dist/server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ dependencies {
compileOnly depLibs.lombok
compileOnly depLibs.spotbugsAnnotations

implementation depLibs.slf4jLog4j
implementation depLibs.log4j12api


testCompileOnly depLibs.lombok
Expand Down
12 changes: 10 additions & 2 deletions bookkeeper-dist/server/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,16 @@

<!-- slf4j binding -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>

<dependency>
Expand Down
8 changes: 5 additions & 3 deletions bookkeeper-dist/src/main/resources/LICENSE-all.bin.txt
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ Apache Software License, Version 2.
- lib/io.vertx-vertx-core-3.9.8.jar [15]
- lib/io.vertx-vertx-web-3.9.8.jar [16]
- lib/io.vertx-vertx-web-common-3.9.8.jar [16]
- lib/log4j-log4j-1.2.17.jar [17]
- lib/org.apache.logging.log4j-log4j-1.2-api-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-api-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-core-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-slf4j-impl-2.14.1.jar [17]
- lib/net.java.dev.jna-jna-3.2.7.jar [18]
- lib/org.apache.commons-commons-collections4-4.1.jar [19]
- lib/org.apache.commons-commons-lang3-3.6.jar [20]
Expand Down Expand Up @@ -318,7 +321,7 @@ Apache Software License, Version 2.
[14] Source available at https://github.com/vert-x3/vertx-bridge-common/tree/3.9.8
[15] Source available at https://github.com/eclipse/vert.x/tree/3.9.8
[16] Source available at https://github.com/vert-x3/vertx-web/tree/3.9.8
[17] Source available at http://logging.apache.org/log4j/1.2/download.html
[17] Source available at https://github.com/apache/logging-log4j2/tree/rel/2.14.1
[18] Source available at https://github.com/java-native-access/jna/tree/3.2.7
[19] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-collections.git;a=tag;h=a3a5ad
[20] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-lang.git;a=shortlog;h=refs/tags/LANG_3_6
Expand Down Expand Up @@ -639,7 +642,6 @@ MIT license. For details, see deps/slf4j-1.7.32/LICENSE.txt.

Bundled as
- lib/org.slf4j-slf4j-api-1.7.32.jar
- lib/org.slf4j-slf4j-log4j12-1.7.32.jar
Source available at https://github.com/qos-ch/slf4j/tree/v_1.7.32
------------------------------------------------------------------------------------
This product bundles the Google Auth Library, which is available under a "3-clause BSD"
Expand Down
8 changes: 5 additions & 3 deletions bookkeeper-dist/src/main/resources/LICENSE-bkctl.bin.txt
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,10 @@ Apache Software License, Version 2.
- lib/io.netty-netty-transport-4.1.68.Final.jar [11]
- lib/io.netty-netty-transport-native-epoll-4.1.68.Final-linux-x86_64.jar [11]
- lib/io.netty-netty-transport-native-unix-common-4.1.68.Final.jar [11]
- lib/log4j-log4j-1.2.17.jar [16]
- lib/org.apache.logging.log4j-log4j-1.2-api-2.14.1.jar [16]
- lib/org.apache.logging.log4j-log4j-api-2.14.1.jar [16]
- lib/org.apache.logging.log4j-log4j-core-2.14.1.jar [16]
- lib/org.apache.logging.log4j-log4j-slf4j-impl-2.14.1.jar [16]
- lib/net.java.dev.jna-jna-3.2.7.jar [17]
- lib/org.apache.commons-commons-collections4-4.1.jar [18]
- lib/org.apache.commons-commons-lang3-3.6.jar [19]
Expand Down Expand Up @@ -290,7 +293,7 @@ Apache Software License, Version 2.
[9] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-lang.git;a=tag;h=375459
[10] Source available at http://svn.apache.org/viewvc/commons/proper/logging/tags/commons-logging-1.1.1/
[11] Source available at https://github.com/netty/netty/tree/netty-4.1.68.Final
[16] Source available at http://logging.apache.org/log4j/1.2/download.html
[16] Source available at https://github.com/apache/logging-log4j2/tree/rel/2.14.1
[17] Source available at https://github.com/java-native-access/jna/tree/3.2.7
[18] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-collections.git;a=tag;h=a3a5ad
[19] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-lang.git;a=shortlog;h=refs/tags/LANG_3_6
Expand Down Expand Up @@ -565,7 +568,6 @@ MIT license. For details, see deps/slf4j-1.7.32/LICENSE.txt.

Bundled as
- lib/org.slf4j-slf4j-api-1.7.32.jar
- lib/org.slf4j-slf4j-log4j12-1.7.32.jar
Source available at https://github.com/qos-ch/slf4j/tree/v_1.7.32
------------------------------------------------------------------------------------
This product bundles the Google Auth Library, which is available under a "3-clause BSD"
Expand Down
8 changes: 5 additions & 3 deletions bookkeeper-dist/src/main/resources/LICENSE-server.bin.txt
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ Apache Software License, Version 2.
- lib/io.vertx-vertx-core-3.9.8.jar [15]
- lib/io.vertx-vertx-web-3.9.8.jar [16]
- lib/io.vertx-vertx-web-common-3.9.8.jar [16]
- lib/log4j-log4j-1.2.17.jar [17]
- lib/org.apache.logging.log4j-log4j-1.2-api-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-api-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-core-2.14.1.jar [17]
- lib/org.apache.logging.log4j-log4j-slf4j-impl-2.14.1.jar [17]
- lib/net.java.dev.jna-jna-3.2.7.jar [18]
- lib/org.apache.commons-commons-collections4-4.1.jar [19]
- lib/org.apache.commons-commons-lang3-3.6.jar [20]
Expand Down Expand Up @@ -316,7 +319,7 @@ Apache Software License, Version 2.
[14] Source available at https://github.com/vert-x3/vertx-bridge-common/tree/3.9.8
[15] Source available at https://github.com/eclipse/vert.x/tree/3.9.8
[16] Source available at https://github.com/vert-x3/vertx-web/tree/3.9.8
[17] Source available at http://logging.apache.org/log4j/1.2/download.html
[17] Source available at https://github.com/apache/logging-log4j2/tree/rel/2.14.1
[18] Source available at https://github.com/java-native-access/jna/tree/3.2.7
[19] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-collections.git;a=tag;h=a3a5ad
[20] Source available at https://git-wip-us.apache.org/repos/asf?p=commons-lang.git;a=shortlog;h=refs/tags/LANG_3_6
Expand Down Expand Up @@ -631,7 +634,6 @@ MIT license. For details, see deps/slf4j-1.7.32/LICENSE.txt.

Bundled as
- lib/org.slf4j-slf4j-api-1.7.32.jar
- lib/org.slf4j-slf4j-log4j12-1.7.32.jar
Source available at https://github.com/qos-ch/slf4j/tree/v_1.7.32
------------------------------------------------------------------------------------
This product bundles the Google Auth Library, which is available under a "3-clause BSD"
Expand Down
4 changes: 3 additions & 1 deletion bookkeeper-server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ dependencies {
testImplementation depLibs.zookeeperTest
annotationProcessor depLibs.lombok
testAnnotationProcessor depLibs.lombok
testImplementation depLibs.slf4jLog4j
testImplementation depLibs.log4jSlf4jImpl
testImplementation depLibs.log4j12api
testImplementation depLibs.log4jCore
}

test {
Expand Down
12 changes: 10 additions & 2 deletions bookkeeper-server/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,16 @@
<artifactId>rocksdbjni</artifactId>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>
<dependency>
<groupId>org.apache.zookeeper</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.apache.commons.cli.Options;
import org.apache.commons.configuration.ConfigurationException;
import org.apache.commons.io.FileUtils;
import org.apache.log4j.ConsoleAppender;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -335,8 +336,9 @@ private static void printHelp(Options opts) {

public static void main(String[] args) throws Exception {
org.apache.log4j.Logger root = org.apache.log4j.Logger.getRootLogger();
root.addAppender(new org.apache.log4j.ConsoleAppender(
new org.apache.log4j.PatternLayout("%-5p [%t]: %m%n")));
ConsoleAppender console = new org.apache.log4j.ConsoleAppender();
console.setLayout(new org.apache.log4j.PatternLayout("%-5p [%t]: %m%n"));
root.addAppender(console);
root.setLevel(org.apache.log4j.Level.ERROR);
org.apache.log4j.Logger.getLogger(FileSystemUpgrade.class).setLevel(
org.apache.log4j.Level.INFO);
Expand Down
Loading

0 comments on commit a522fa3

Please sign in to comment.