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

Tolerate unprivileged log4j getClassLoaders calls #81840

Merged
merged 8 commits into from
Dec 18, 2021
Merged

Tolerate unprivileged log4j getClassLoaders calls #81840

merged 8 commits into from
Dec 18, 2021

Conversation

ChrisHegarty
Copy link
Contributor

@ChrisHegarty ChrisHegarty commented Dec 16, 2021 •
edited
Loading

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

@ChrisHegarty ChrisHegarty changed the title WIP - Tolerate un privileged log4j getClassLoaders WIP - Tolerate unprivileged log4j getClassLoaders calls Dec 17, 2021
@ChrisHegarty ChrisHegarty added Team:Core/Infra Meta label for core/infra team auto-backport Automatically create backport pull requests when merged v6.8.22 v7.16.2 v8.0.0 labels Dec 17, 2021
@ChrisHegarty ChrisHegarty changed the title WIP - Tolerate unprivileged log4j getClassLoaders calls Tolerate unprivileged log4j getClassLoaders calls Dec 17, 2021
@ChrisHegarty ChrisHegarty marked this pull request as ready for review December 17, 2021 14:34
@elasticmachine
Copy link
Collaborator

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

@ChrisHegarty ChrisHegarty added the :Core/Infra/Core Core issues without another label label Dec 17, 2021
pgomulka
pgomulka approved these changes Dec 17, 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 to me, I don't know how to test this in a unit test though..

@pgomulka
Copy link
Contributor

@nik9000 and @tvernum can you also please take a look?

@ChrisHegarty ChrisHegarty merged commit 4cc56de into elastic:master Dec 18, 2021
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.0 Commit could not be cherrypicked due to conflicts
❌ 7.16 Commit could not be cherrypicked due to conflicts
❌ 6.8 Commit could not be cherrypicked due to conflicts
❌ 7.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 81840

@ChrisHegarty ChrisHegarty mentioned this pull request Dec 18, 2021
Merged
@ChrisHegarty ChrisHegarty deleted the tolerate_getClassLoaders branch December 18, 2021 17:17
@ChrisHegarty ChrisHegarty mentioned this pull request Dec 18, 2021
Merged
ChrisHegarty added a commit that referenced this pull request Dec 18, 2021
@ChrisHegarty
[6.8.22] Upgrade to log4j 2.17.0 (#81906)
71fcb50 
Backport of:

Upgrade to log4j 2.17.0 Upgrade to log4j 2.17.0 #81902
Tolerate unprivileged log4j getClassLoaders calls Tolerate unprivileged log4j getClassLoaders calls #81840
Tolerate benign log4j status messages in tests Tolerate benign log4j status messages in tests #81851
@tvernum
Copy link
Contributor

tvernum commented Dec 18, 2021

LGTM.
Can we restrospectively add a test that other code can't call getClassLoaders?
The positive case is exercised by integ tests, but I don't see anything that would catch a case where we inadvertently made the check too permissive.

ChrisHegarty added a commit to ChrisHegarty/elasticsearch that referenced this pull request Dec 20, 2021
@ChrisHegarty
Tolerate unprivileged log4j getClassLoaders calls (elastic#81840)
d62af82 
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.
@ChrisHegarty ChrisHegarty mentioned this pull request Dec 20, 2021
Merged
ChrisHegarty added a commit that referenced this pull request Dec 20, 2021
@ChrisHegarty
Tolerate unprivileged log4j getClassLoaders calls (#81840) (#81914)
4299e3e 
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.
@pgomulka pgomulka mentioned this pull request Dec 20, 2021
Merged
pgomulka added a commit that referenced this pull request Dec 20, 2021
@pgomulka
[7.16] Tolerate unprivileged log4j getClassLoaders calls (#81903) (#8…
a1b144d 
…1923)

Backport of

Tolerate unprivileged log4j getClassLoaders calls Tolerate unprivileged log4j getClassLoaders calls #81840
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

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v6.8.22 v7.16.2 v7.17.0 v8.0.0-rc1 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ChrisHegarty @elasticmachine @pgomulka @elasticsearchmachine @tvernum @mark-vieira