Skip to content

Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 #2816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zymap merged 11 commits into from
Oct 15, 2021
Merged

Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 #2816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
14d3a4c
Change dependencies for log4j2.
RaulGracia Sep 30, 2021
49a33e2
Fixed most tests and references to internal log4j1.2 APIs.
RaulGracia Oct 6, 2021
c201c5a
Fix PR validation
RaulGracia Oct 6, 2021
983737f
Fixed license checks.
RaulGracia Oct 6, 2021
210e032
Restore slf4j simple in class loader.
RaulGracia Oct 6, 2021
adbd5f5
Using slf4j MDC API.
RaulGracia Oct 7, 2021
273da9e
Remove unnecessary lib exclusions.
RaulGracia Oct 7, 2021
265a957
Working on gradle dependencies.
RaulGracia Oct 7, 2021
5fd2b53
Fix import order.
RaulGracia Oct 7, 2021
221c892
Revert "Remove unnecessary lib exclusions."
RaulGracia Oct 7, 2021
48cbaed
Remove transient addition of log4j1.2 from hadoop library.
RaulGracia Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions bookkeeper-common/build.gradle
View file
Open in desktop
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
View file
Open in desktop
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>

@eolivelli eolivelli Oct 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should not add these deps in common.
The common and server packages should not bring in any implementation

</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
2 changes: 1 addition & 1 deletion bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
View file
Open in desktop
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
40 changes: 22 additions & 18 deletions ...common/src/test/java/org/apache/bookkeeper/common/util/TestOrderedExecutorDecorators.java
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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>

@pkumar-singh pkumar-singh Oct 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I am late at the PR, apologies. But I am not sure this is the right thing to do. Bookkeeper should work with SLF4J instead of log4j. Allow me to explain. There are three elements to it.

  1. SLF4J this is the facade for logging framework. It does not provide logging implementation. But just facade API. Bookkeeper should only be using that, test code could be a possible exception. That way SLF4j can work with any type of logging infra, log4j2 or log4j1 being just few options out of many.
  2. SLF4j-LOG4J12: This is Binding for SLF4 facade with LOG4J1, "ideally" bookkeeper should not have any dependency on this. This should again be provided by application at runtime. Role of SLF4j-LOG4J12 is that If SLF4J sees SLF4j-LOG4J12 in the Runtime classpath, It will go on lookout for log4j12. And that will ensure that for actual logging log4j2 or log4j12 is used. In other words SLF4j-LOG4J12 works as bridge between facade and implementation.
  3. log4j12 or log4j2 or simplelog4j or foolog4j: This is actual logging library. Which does the actual logging. And certainly should be provided at runtime by application. In ideal case Bookkeeper should not provide this as dependency.

To me looks like we have removed slf4j and started using log4j directly. Which means we have created a hard binding with log4j, which I believe is not desirable.

@eolivelli @RaulGracia @merlimat @ivankelly @hsaputra

@eolivelli eolivelli Oct 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the dist package.

It is important that in BK code we use only slf4j

But this is the package, we have to provide an implementation

<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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 4 additions & 2 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java
View file
Open in desktop
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