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

CVE-2022-34917 (High) detected in kafka-clients-3.0.1.jar #2117

Closed
wants to merge 0 commits into from

Conversation

vinayak15
Copy link
Contributor

@vinayak15 vinayak15 commented Sep 28, 2022

Description

[Describe what this change achieves]
Update Kafka version to fix security vulnerability(#2095)

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@vinayak15 vinayak15 requested a review from a team September 28, 2022 17:25
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Happy to merge after we get a resolution on the change around jarhell

build.gradle Outdated
@@ -98,7 +98,6 @@ forbiddenApisTest.enabled = false
filepermissions.enabled = false
forbiddenPatterns.enabled = false
testingConventions.enabled = false
// Conflicts between runtime kafka-clients:3.0.1 & testRuntime kafka-clients:3.0.1:test
Copy link
Member

Choose a reason for hiding this comment

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

This comment reference to the line below, can we activate jarhell with 3.0.2?

Copy link
Member

Choose a reason for hiding this comment

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

@peternied The kafka change to resolve jar hell was merged into main and didn't make it into the 3.0.2 release.

Here's the change merged into main: apache/kafka#12407

Looks like its still in the code of 3.0.2: https://github.com/apache/kafka/blob/25b1aea02e37da1457026f6ac82a66f81c878e45/build.gradle#L1261

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but we should keep the comment until we've resolved the issue it mentions. If the concern is about the version number being hardcoded to a different version, you could just delete them or replace them with X.Y.Z.

Copy link
Member

Choose a reason for hiding this comment

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

Yes we should leave the comment in until the jar hell issue is resolved

build.gradle Outdated
@@ -307,7 +306,7 @@ dependencies {
}
implementation 'com.github.wnameless:json-flattener:0.5.0'
implementation 'com.flipkart.zjsonpatch:zjsonpatch:0.4.4'
implementation 'org.apache.kafka:kafka-clients:3.0.1'
implementation 'org.apache.kafka:kafka-clients:3.0.2'
Copy link
Member

Choose a reason for hiding this comment

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

Can we make a variable for the kafka version so future changes don't need to update so many lines at once?

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2022

Codecov Report

Merging #2117 (607c07b) into main (2a00e2b) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 607c07b differs from pull request most recent head 3b16bf2. Consider uploading reports for the commit 3b16bf2 to get more accurate results

@@             Coverage Diff              @@
##               main    #2117      +/-   ##
============================================
- Coverage     61.04%   61.01%   -0.03%     
+ Complexity     3230     3229       -1     
============================================
  Files           256      256              
  Lines         18077    18077              
  Branches       3224     3224              
============================================
- Hits          11035    11030       -5     
- Misses         5468     5471       +3     
- Partials       1574     1576       +2     
Impacted Files Coverage Δ
...urity/ssl/transport/SecuritySSLNettyTransport.java 62.36% <0.00%> (-4.31%) ⬇️
...earch/security/ssl/util/SSLConnectionTestUtil.java 93.18% <0.00%> (-2.28%) ⬇️
...search/security/transport/SecurityInterceptor.java 76.15% <0.00%> (+0.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cwperks
Copy link
Member

cwperks commented Sep 29, 2022

@peternied Do you know if there's anything preventing us from upgrading to the latest version of this jar. I see v3.2.3 published on September 17 in maven: https://mvnrepository.com/artifact/org.apache.kafka/kafka-clients

@vinayak15 There was a fairly big change in the build.gradle file between 1.3 and 2.x so I'm not sure if the backport PR will work automatically. We may need to manually cut a PR against the 1.3 branch with an upgrade in vulnerable dependencies.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Ty @vinayak15. Please rebase and sign-off the latest commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants