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.

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

Jump to bottom

Upgrade log4j to 2.16.0 #81759

Closed
wants to merge 2 commits into from
Closed

Upgrade log4j to 2.16.0 #81759

wants to merge 2 commits into from

Conversation

arteam
Copy link
Contributor

@arteam arteam commented Dec 15, 2021 •
edited
Loading

We upgraded log4j to 2.15.0 in #81709 and we can do the next upgrade. It makes JNDI lookups opt-in and also fixes CVE-2021-45046

This also reverts #81629 where we removed the JndiLookup class from the log4j jar.

@arteam
Upgrade log4j to 2.16.0
98656eb 
We upgraded log4j to 2.15.0 in elastic#81709 and we can do the next upgrade.
It makes JNDI opt-in and also fixes [CVE-2021-45046](GHSA-7rjr-3q55-vv33)
@arteam arteam added the :Core/Infra/Logging Log management and logging utilities label Dec 15, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Dec 15, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

mark-vieira
mark-vieira approved these changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

@arteam arteam mentioned this pull request Dec 15, 2021
Merged
pgomulka
pgomulka suggested changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

it looks good. I would prefer to split a revert and upgrade into separate PRs.
There should also be a revert of ef64808 before we upgrade to 2.16?

also it looks like there should be another log4j release very soon
apache/logging-log4j2@4456909#r61971917

distribution/build.gradle
@@ -275,10 +275,6 @@ configure(subprojects.findAll { ['archives', 'packages'].contains(it.name) }) {
}
}
}
all {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this in a separate PR.
Possibly a simple revert of this whole commit 9a3422e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me extract the revert into a separate PR and then we can rebase this PR when the revert gets merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted as #81783

@jasvinder1107
Copy link

jasvinder1107 commented Dec 15, 2021 •
edited
Loading

@arteam can you please look at why merging is blocked? This is high priority fix that we need to patch this before year end.

@arteam arteam mentioned this pull request Dec 15, 2021
Closed
@jasvinder1107
Copy link

@arteam thank you for so quick response. @pgomulka please give your blessing on this ps so this can be merged. :)

@jasvinder1107
Copy link

jasvinder1107 commented Dec 16, 2021 •
edited
Loading

@arteam @pgomulka I am sorry of being chatty here, can you please help us by merging this asap ?

@DJRickyB
Copy link
Contributor

closes #81867

@ChrisHegarty
Copy link
Contributor

We're already at 2.15.0 and have stripped JnidLookup.class, which is arguably more secure than 2.16.0. The stripping greatly outweighs the benefits of 2.16.0. The 2.16.0 release has a static dependency on JndiLookup.class. We're awaiting 2.6.1 where will be keep the stripping.

ChrisHegarty
ChrisHegarty requested changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

Please do not merge this PR, we're waiting for 2.16.1

@jasvinder1107
Copy link

@ChrisHegarty so just to understand you correctly there will be stripping of the jndiclass from 2.16.1 also ?. Also can you please give us light on CVE in the 2.16.0 ?

@grcevski grcevski mentioned this pull request Dec 17, 2021
Closed
@dionrhys
Copy link

Log4j2 2.17.0 has now been released with a fix to CVE-2021-45105, opt-in system properties for JNDI, and removing support for LDAP(S) over JNDI.

@JafarAkhondali JafarAkhondali mentioned this pull request Dec 18, 2021
Open
@costin
Copy link
Member

costin commented Dec 18, 2021

Superseded by #81902 (Upgrade to Log4J 2.17.0)

@costin costin closed this Dec 18, 2021
@arteam arteam deleted the upgrade-log4j-2.16.0 branch March 24, 2022 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisHegarty ChrisHegarty ChrisHegarty requested changes

@pgomulka pgomulka pgomulka requested changes

@jasvinder1107 jasvinder1107 jasvinder1107 approved these changes

@mark-vieira mark-vieira mark-vieira approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Logging Log management and logging utilities dependencies >non-issue Team:Core/Infra Meta label for core/infra team v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@arteam @elasticmachine @jasvinder1107 @DJRickyB @ChrisHegarty @dionrhys @costin @mark-vieira @pgomulka