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

HIVE-28164: Remove log4j:log4j transitive dependency #5172

Closed
wants to merge 1 commit into from

Conversation

Aggarwal-Raghav
Copy link
Contributor

@Aggarwal-Raghav Aggarwal-Raghav commented Mar 29, 2024

What changes were proposed in this pull request?

There are dependency like accumulo and slf4j bringing log4j vulnerable jars in dependency tree. There are few dependencies also which don't appear in dependecy tree but are bringing old and vulnerable log4j. This can be observed in local m2 repo cache.

Why are the changes needed?

For CVE's and security reasons.

Does this PR introduce any user-facing change?

NO

Is the change a dependency upgrade?

Yes, here is new dependency tree on 771b003 (master):
dependency_tree.txt

How was this patch tested?

Will see the UT from CI

@Aggarwal-Raghav
Copy link
Contributor Author

@zabetak, if you have bandwidth can you please review and provide your inputs on this?

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

If we want to eliminate log4j completely from the project then we should also tune the respective enforcer rules to prevent this from reappearing.

<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-container-default</artifactId>
<version>1.5.6</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use properties instead of hardcoded versions.

@Aggarwal-Raghav
Copy link
Contributor Author

Apologies for the late response, Have updated the PR with suggested changes. I have created a new enforcer execution step to search for transitive dependencies of log4j. The scope of this PR is to only exclude log4j.

Copy link

sonarcloud bot commented Apr 26, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Aggarwal-Raghav
Copy link
Contributor Author

@zabetak , can you please help with review?

beeline/pom.xml Outdated Show resolved Hide resolved
@Aggarwal-Raghav
Copy link
Contributor Author

@zabetak , can you please re-review it, if you have the bandwidth

Copy link

github-actions bot commented Oct 4, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the [email protected] list if the patch is in need of reviews.

@github-actions github-actions bot added the stale label Oct 4, 2024
@zabetak
Copy link
Contributor

zabetak commented Oct 7, 2024

I forgot about this issue. I will take a a look now.

@github-actions github-actions bot removed the stale label Oct 8, 2024
Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

@Aggarwal-Raghav I left some small comments and then we can merge this. Let me know if you have time to address them otherwise I can commit the final touches.

beeline/pom.xml Outdated Show resolved Hide resolved
standalone-metastore/metastore-server/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@Aggarwal-Raghav
Copy link
Contributor Author

Thanks @zabetak , for the review. your insights are always appreciated. I will address the review comments :-)

* Bump accumulo version to 1.10.4
* Exclude log4j:log4j from slf4j-log4j12 in standalone-metastore
Copy link

sonarcloud bot commented Oct 15, 2024

@Aggarwal-Raghav
Copy link
Contributor Author

@zabetak, have updated the PR, can you please review?

Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

LGTM, will merge soon!

@zabetak zabetak closed this in 7e33d79 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants