[FLINK-32176] Exclude snakeyaml from pulsar-client-all to mitigate CVE-2022-1471 #51
Conversation
Samrat002
changed the title
|
Can anyone Please help review the changes |
There was a problem hiding this comment.
IIRC CVE-2022-1471 is argued to be a false positive https://bitbucket.org/snakeyaml/snakeyaml/issues/561/cve-2022-1471-vulnerability-in.
You may suppress the warning instead of eagerly excluding libs.
I can see that this dependency is explicit included in pulsar-client-all:
<include>org.yaml:snakeyaml</include>May you provide evidence that "excluding snakeyaml doesn't effect the client usage in flink-connector-pulsar"?
If it should not be in the fat jar at all, you can submit a PR to Pulsar upstream and we reduce this dependency by upgrade pulsar version.
| @@ -81,6 +81,13 @@ under the License. | |||
| <dependency> | |||
| <groupId>org.apache.pulsar</groupId> | |||
| <artifactId>pulsar-client-all</artifactId> | |||
| <exclusions> | |||
There was a problem hiding this comment.
We don't need this exclusion.
| @@ -181,6 +181,10 @@ under the License. | |||
| <artifactId>pulsar-client-all</artifactId> | |||
| <version>${pulsar.version}</version> | |||
| <exclusions> | |||
| <exclusion> | |||
There was a problem hiding this comment.
We don't need this exclusion.
|
@Samrat002 IIUC, you want to remove the snakeyaml dependencies in the flink-connector-pulsar. But I don't think this PR works. All the snakeyaml classes has been shaded in the fat-jar of pulsar-client-all. You can't remove it from pulsar-client-all by simply using a |
Co-authored-by: Yufan Sheng <syhily@gmail.com>
|
Thank you @tisonkun, @syhily taking time in reviewing the small change. my intension was to exclude snakeyaml and mitigate vulnerablity, I missed to see the added unnecessary dependencies creating technical debt in future upgrades.
Yes, i can submit a pr in pulsar upstream and reduce this dependency by upgrading the pulsar version. This would be cleaner way.
I have one query regarding this, Do we really need pulsar client all here in flink connector pulsar ? |
I'm quite willing to answer this question because a lot of people ask the same question while a lot of the hidden details is not well documented. Pulsar has two kinds of the Java client, the admin API and the client API. We are planning to drop the admin API usage because it just exposes a lot of dangerous operations to the end users and a lot of permission issues occurred. But this takes a long time on accomplishing the pulsar client. We are still on the way. |
|
I think we can just close this PR and wait an upstream client bumping. WDYT? @tisonkun @Samrat002 |
|
Thank you @syhily, explanation answers my query 💯 , and gives insight why we are having pulsar-client-all dependency in the connector. I am not familiar with Apache Pulsar and never explored but wish to do it soon and use it in production, it would be great if you want to fix it in Apache Pulsar.
Sure, I will close the PR as it is not relevant and doesn't solve the issue. |
|
It looks like pulsar is now on version |
|
@rorynickolls-skyral We are planning to use the LTS version of the Pulsar client. But the ci can't be passed cause some strange test issues |
Purpose of the change
Mitigate the imapact of CVE in flink
Brief change log
Exclude snakeyaml from pulsar-client-all
Verifying this change
Significant changes
No significant changes
(Please check any boxes [x] if the answer is "yes". You can first publish the PR and check them afterwards, for
convenience.)
@Public(Evolving))