Skip to content
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.

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

[NO-ISSUE] Use reload4j instead of log4j #4719

Merged
merged 7 commits into from
Mar 10, 2024
Merged

Conversation

pjfanning
Copy link
Contributor

What is this PR for?

  • reload4j does not prevent us moving to log4jv2 later
  • reload4j fixes some security issues in log4j
  • it doesn't feel great for an ASF project to still be using insecure logging frameworks like log4j v1

What type of PR is it?

Bug Fix
Improvement
Feature
Documentation
Hot Fix
Refactoring
Please leave your type of PR only

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • Strongly recommended: add automated unit tests for any new or changed behavior
  • Outline any manual steps to test the PR here.

Screenshots (if appropriate)

Questions:

  • Does the license files need to update?
  • Is there breaking changes for older versions?
  • Does this needs documentation?

@pjfanning
Copy link
Contributor Author

pjfanning commented Feb 19, 2024

@jongyoul is this something that could be merged? I know log4jv2 is under consideration but log4jv1 is a really bad thing to have a dependency and it is much simpler to use reload4j. Some of the big Zeppelin dependencies already use reload4j - making it more complicated for Zeppelin to use log4jv2.

@pjfanning pjfanning changed the title [DRAFT] use reload4j use reload4j instead of log4j Feb 19, 2024
@jongyoul jongyoul changed the title use reload4j instead of log4j [NO-ISSUE] Use reload4j instead of log4j Feb 20, 2024
jongyoul
jongyoul previously approved these changes Feb 20, 2024
@@ -114,8 +114,7 @@
<plugin.frontend.version>1.12.1</plugin.frontend.version>

<!-- common library versions -->
<slf4j.version>1.7.30</slf4j.version>
<log4j.version>1.2.17</log4j.version>
<slf4j.version>1.7.35</slf4j.version>
Copy link
Member

Choose a reason for hiding this comment

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

why not 1.7.36?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

causes a maven convergence issue - due to other dependencies of zeppelin using 1.7.35

I can try further changes but it could take a few days.

The key thing is to get agreement that keeping log4jv1 is not a good thing and that reload4j is a reasonable short term change to avoid using a completely insecure logging framework.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the elaboration, that makes sense.

@@ -240,12 +239,6 @@
<version>${slf4j.version}</version>
</dependency>

<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

removing the definition from dependencyManagement does not mean excluding the jar from the final binary tarball, there is still a chance that log4j is collected as the transitive dependency into the final tarball, additional manual checks are required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a better look over the next day or 2 to see what transitive dependencies could leaking log4j dependencies into zeppelin. There already appears to be issues (pre-existing issues unrelated to this change) where log4j and log4j-1.2-api are conflicting. log4j-1.2-api is a log4jv2 jar that pretends to be log4jv1 and you shouldn't have this on the classpath at the same time as you have the legacy log4j jar.

zeppelin-server/pom.xml Outdated Show resolved Hide resolved
@pjfanning
Copy link
Contributor Author

I built zeppelin-distribution/target/zeppelin-0.11.0-SNAPSHOT.tar.gz locally with these changes and log4j jar was not in the dist.

@pjfanning
Copy link
Contributor Author

pjfanning commented Mar 10, 2024

@jongyoul @pan3793 @Reamer could this PR be merged? The ASF is suffering damage to our reputation by having project's who won't use secure libraries - in particular old versions of log4j make us look really bad.

@jongyoul jongyoul merged commit d0a71c7 into apache:master Mar 10, 2024
31 checks passed
HritwikSinghal pushed a commit to HritwikSinghal/zeppelin that referenced this pull request Mar 11, 2024
jongyoul pushed a commit that referenced this pull request Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants