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 org.apache.logging.log4j:log4j-core and log4j-api libraries #24507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Upgrade org.apache.logging.log4j:log4j-core and log4j-api libraries #24507

wants to merge 1 commit into from
+15 −6

Conversation

Dilli-Babu-Godari
Copy link
Contributor

@Dilli-Babu-Godari Dilli-Babu-Godari commented Feb 6, 2025 •
edited
Loading

Description

Upgraded org.apache.logging.log4j:log4j-core from 2.17.1 to 2.24.3
Upgraded org.apache.logging.log4j:log4j-api from 2.17.1 to 2.24.3

Motivation and Context

Addresses below CVEs

Screenshot 2025-02-06 at 4 55 39 PM

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our co
    ntributing guide    , in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
*  Upgraded org.apache.logging.log4j:log4j-core from 2.17.1 to 2.24.3 in response to `CVE-2024-47554<https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-47554>`
*  Upgraded org.apache.logging.log4j:log4j-api from 2.17.1 to 2.24.3 in response to `CVE-2024-47554 <https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-47554>`

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Feb 6, 2025
@Dilli-Babu-Godari Dilli-Babu-Godari marked this pull request as ready for review February 6, 2025 12:24
@Dilli-Babu-Godari Dilli-Babu-Godari requested a review from a team as a code owner February 6, 2025 12:24
imjalpreet
imjalpreet requested changes Feb 6, 2025
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@Dilli-Babu-Godari, I noticed that these libraries are also included with version 2.17.1 in presto-druid. Is there a specific reason we’re not upgrading them as well? Could you take a look?

@Dilli-Babu-Godari
Copy link
Contributor Author

Dilli-Babu-Godari commented Feb 7, 2025 •
edited
Loading

@Dilli-Babu-Godari, I noticed that these libraries are also included with version 2.17.1 in presto-druid. Is there a specific reason we’re not upgrading them as well? Could you take a look?

I have now added the presto-druid as well. Could you please review it again?

agrawalreetika
agrawalreetika requested changes Feb 7, 2025
View reviewed changes
Copy link
Member

@agrawalreetika agrawalreetika left a comment

Choose a reason for hiding this comment

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

@Dilli-Babu-Godari Please verify both the dependency by checking the dependency tree, I still see these 2 are coming from some more packages.

@Dilli-Babu-Godari Dilli-Babu-Godari force-pushed the log4j_upgrade branch 2 times, most recently from 0016219 to a0993dc Compare February 11, 2025 03:48
@Dilli-Babu-Godari
Copy link
Contributor Author

@Dilli-Babu-Godari Please verify both the dependency by checking the dependency tree, I still see these 2 are coming from some more packages.

I have made changes, so the versions are now aligned.
Screenshot 2025-02-11 at 10 08 17 AM

@steveburnett
Copy link
Contributor

In the release note entries, please add a link to a CVE. See Phrasing in the Release Notes Guidelines for an example and formatting. Because there are so many CVEs shown in this one, choose an appropriate CVE - perhaps the most recent one?

imjalpreet
imjalpreet requested changes Feb 11, 2025
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@Dilli-Babu-Godari thanks for the changes. Can you also confirm the output of the below commands:

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-api

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-core

presto-druid/pom.xml
Copy link
Member

Choose a reason for hiding this comment

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

Since we have moved the dependencies to root pom, we can remove the version in this pom for both the dependencies.

presto/presto-druid/pom.xml

Line 181 in e84e4d2

<version>${dep.log4j.version}</version>

presto/presto-druid/pom.xml

Line 188 in e84e4d2

<version>${dep.log4j.version}</version>

presto-elasticsearch/pom.xml
Copy link
Member

Choose a reason for hiding this comment

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

Since we have moved the dependencies to root pom, we can remove the version in this pom as well for both the dependencies.

presto/presto-elasticsearch/pom.xml

Line 183 in e84e4d2

<version>${dep.log4j.version}</version>

presto/presto-elasticsearch/pom.xml

Line 176 in e84e4d2

<version>${dep.log4j.version}</version>

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 have removed the version tag. Could you please review again ?

Copy link
Member

Choose a reason for hiding this comment

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

Can you also confirm the output of the below commands:

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-api

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-core

@Dilli-Babu-Godari Can you also share these?

@Dilli-Babu-Godari
Upgrade org.apache.logging.log4j:log4j-core and log4j-api libraries i…
792118c 
…n presto

Upgraded org.apache.logging.log4j:log4j-core from 2.17.1 to 2.24.3
Upgraded org.apache.logging.log4j:log4j-api from 2.17.1 to 2.24.3

Fixes almost 25 CVEs.
@steveburnett
Copy link
Contributor

In the release note entries, please add a link to a CVE. See Phrasing in the Release Notes Guidelines for an example and formatting. Because there are so many CVEs shown in this one, choose an appropriate CVE - perhaps the most recent one?

Thank you!

@Dilli-Babu-Godari
Copy link
Contributor Author

@Dilli-Babu-Godari thanks for the changes. Can you also confirm the output of the below commands:

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-api

./mvnw dependency:tree -Dincludes=org.apache.logging.log4j:log4j-core

Screenshot 2025-02-12 at 10 06 35 PM Screenshot 2025-02-12 at 5 25 40 PM

Build is successful and the modules are having respective upgraded versions of log4j-core and log4j-api

imjalpreet
imjalpreet approved these changes Feb 12, 2025
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@Dilli-Babu-Godari LGTM, I am approving the PR but I have one minor request. The commit message exceeds the single-line character limit. Please update it to the following::

Upgrade log4j-core and log4j-api dependencies

Upgrade org.apache.logging.log4j:log4j-core from 2.17.1 to 2.24.3
Upgrade org.apache.logging.log4j:log4j-api from 2.17.1 to 2.24.3

Fixes almost 25 CVEs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjalpreet imjalpreet imjalpreet approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@agrawalreetika agrawalreetika Awaiting requested review from agrawalreetika

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
from:IBM PR from IBM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Dilli-Babu-Godari @steveburnett @imjalpreet @agrawalreetika @prestodb-ci