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

Upgrade to Log4J 2.18.0 #88237

Merged
merged 2 commits into from
Jul 4, 2022
Merged

Conversation

ChrisHegarty
Copy link
Contributor

Upgrade to log4J 2.18.0.

Remove two workarounds that are no longer needed (since the issues are not present in 2.18.0):

  1. Temporarily provide SystemPropertiesPropertySource Temporarily provide SystemPropertiesPropertySource #87149
  2. The effective granting of permission to LoaderUtil::getClassLoaders

@ChrisHegarty ChrisHegarty added >refactoring Team:Core/Infra Meta label for core/infra team labels Jul 3, 2022
@ChrisHegarty ChrisHegarty self-assigned this Jul 3, 2022
@ChrisHegarty ChrisHegarty changed the title Upgrade to log4J 2.18.0 Upgrade to Log4J 2.18.0 Jul 3, 2022
@ChrisHegarty ChrisHegarty marked this pull request as ready for review July 3, 2022 14:53
@elasticmachine
Copy link
Collaborator

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

@ChrisHegarty ChrisHegarty added :Core/Infra/Core Core issues without another label >upgrade and removed >refactoring labels Jul 3, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @ChrisHegarty, I've created a changelog YAML for you.

@ChrisHegarty ChrisHegarty requested a review from pgomulka July 3, 2022 15:30
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,
I left one question to understand the underlying problem

@@ -124,11 +103,7 @@ public boolean implies(ProtectionDomain domain, Permission permission) {
if ("<<ALL FILES>>".equals(permission.getName())) {
hadoopHack();
}
} else if (permission instanceof RuntimePermission
&& "getClassLoader".equals(permission.getName())
&& isLoaderUtilGetClassLoaders()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand. THe clue of the problem was the getParent which did not have the permission?
https://issues.apache.org/jira/browse/LOG4J2-3236

Just calling LoaderUtil.getClassLoader is safe (I suppose yes, but can you do you know why)? because it is still used
https://github.com/apache/logging-log4j2/blob/log4j-2.18.0-rc1/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java#L266

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... let me recheck this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that LoaderUtil::getClassLoader is still present (in 2.18.0), and we can potentially suffer from the getParent issue which we ran into before. But it seems that all the call flows and tests do not encounter it. Since there are no plans to back port this, and all seems ok, I propose that we merge the change as is, but reserve the right to re-instate the workaround in ESPolicy if we encounter issues in the future.

@ChrisHegarty ChrisHegarty merged commit 453f12c into elastic:master Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team >upgrade v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants