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.15.0 #81709

Merged
merged 7 commits into from Dec 15, 2021
Merged

Upgrade log4j to 2.15.0 #81709

merged 7 commits into from Dec 15, 2021

Conversation

arteam
Copy link
Contributor

@arteam arteam commented Dec 14, 2021

Originally we tried to a log4j update in #47298, but we were unable to that due to the DeprecationLoggerTests.testLogPermissions test failing. The test relied on mocking and got removed as part of refactoring in
#61474.

Now we should be able to the upgrade and then we can address the Security Manager permission questions raised in #47298 separately.

@pgomulka @arteam
Upgrade log4j to 2.15.0
1089bd8 
Originally we tried to a log4j update in elastic#47298, but we were unable to
that due to the `DeprecationLoggerTests.testLogPermissions` test
failing. The test relied on mocking and got removed in
https://github.com/elastic/elasticsearch/pull/61474/files#diff-70de5a6ba5c637e7f19c51341417760d6e957beb5a1fa5703049095ea2719ee0L47

Now we should be able to the upgrade and then we can address the Security
Manager permission questions raised in elastic#47298 separately.
@arteam arteam added :Core/Infra/Logging Log management and logging utilities v8.0.0 dependencies auto-backport Automatically create backport pull requests when merged labels Dec 14, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Dec 14, 2021
@elasticmachine
Copy link
Collaborator

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

pgomulka
pgomulka requested changes Dec 14, 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.

left few questions

plugins/discovery-gce/build.gradle Show resolved Hide resolved
plugins/repository-gcs/build.gradle Outdated Show resolved Hide resolved
plugins/repository-s3/build.gradle Outdated Show resolved Hide resolved
x-pack/plugin/core/build.gradle Show resolved Hide resolved
ChrisHegarty
ChrisHegarty approved these changes Dec 14, 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.

LGTM

@arteam arteam added the v7.16.2 label Dec 14, 2021
pgomulka
pgomulka approved these changes Dec 14, 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.

LGTM, thank you !

@arteam
Copy link
Contributor Author

arteam commented Dec 14, 2021

@elasticmachine update branch

@ChrisHegarty ChrisHegarty self-requested a review December 14, 2021 17:53
@wallrik
Copy link
Contributor

wallrik commented Dec 14, 2021

When you work on this pull request, if you look at Log4j 2.16.0 instead, it looks like the hack from #81629 can probably be removed, if that is something you'd want to do.

@Xon
Copy link

Xon commented Dec 14, 2021 •
edited

When you work on this pull request, if you look at Log4j 2.16.0 instead, it looks like the hack from #81629 can probably be removed, if that is something you'd want to do.

2.15.0 is affected by CVE-2021-45046 to boot, which is thankfully migrated already by the previous removing of JndiLookup class

@arteam arteam force-pushed the reincarnate-log4j2-update branch 2 times, most recently from cedefa2 to 3811045 Compare December 14, 2021 20:11
@mark-vieira
Copy link
Contributor

FYI we should revert #81629 as part of this as well as it's no longer necessary, especially if we go strait to 2.16 which disabled JNDI support entirely by default.

@Li4n0
Copy link

Li4n0 commented Dec 15, 2021

log4j-core has released version 2.16.0, which fixes a denial-of-service attack vulnerability in 2.15.0, would you consider upgrading directly to 2.16.0? @arteam

@arteam arteam force-pushed the reincarnate-log4j2-update branch 2 times, most recently from adffd20 to 3811045 Compare December 15, 2021 08:34
@arteam
Copy link
Contributor Author

arteam commented Dec 15, 2021 •
edited

@Li4n0 We will do the 2.15.0 to 2.16.0 upgrade separately. Unfortunately, it's not a simple version bump, because 2.16.0 forbids things like ${sys:es.logs.cluster_name} in pattern layouts which are used in Elasticsearch

EDIT: It seems the failures were caused by stripping the JNDILookup class in 2.16.0, #81759 should be a clean upgrade.

@arteam arteam force-pushed the reincarnate-log4j2-update branch 3 times, most recently from ba9b1be to ef44c85 Compare December 15, 2021 11:14
@arteam arteam added v8.0.0 and removed auto-backport Automatically create backport pull requests when merged v8.0.0 v7.16.2 labels Dec 15, 2021
@arteam arteam merged commit 442a13a into elastic:master Dec 15, 2021
@arteam arteam deleted the reincarnate-log4j2-update branch December 15, 2021 13:31
@arteam
Copy link
Contributor Author

arteam commented Dec 15, 2021

Thanks @ChrisHegarty and @pgomulka!

arteam added a commit to arteam/elasticsearch that referenced this pull request Dec 15, 2021
@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 mentioned this pull request Dec 15, 2021
Closed
@ChrisHegarty ChrisHegarty mentioned this pull request Dec 15, 2021
Closed
@arteam arteam mentioned this pull request Dec 15, 2021
Closed
This was referenced Dec 17, 2021
Open
Merged
ChrisHegarty added a commit that referenced this pull request Dec 18, 2021
@ChrisHegarty
Tolerate unprivileged log4j getClassLoaders calls (#81840)
4cc56de 
Tolerate unprivileged log4j getClassLoaders calls, as if (but not exactly) like
they were wrapped in doPriv. This is precautionary step as security permission
exceptions have been observed during testing.

This change also reverts changes to tests in the log4j 2.15 Upgrade #81709,
as they should no longer be needed, given this change and changes in #81851.

No explicit new test has been added in this PR, but the code in question is
exercised extensively by existing tests, since the policy is set in the test
framework. The test reverts mentioned above confirm that the changes are
working as expected.

This change is a workaround to the issue raised in log4j:
https://issues.apache.org/jira/browse/LOG4J2-3236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgomulka pgomulka pgomulka approved these changes

@ChrisHegarty ChrisHegarty Awaiting requested review from ChrisHegarty

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.

None yet
9 participants
@arteam @elasticmachine @wallrik @Xon @mark-vieira @Li4n0 @pgomulka @ChrisHegarty @elasticsearchmachine