Upgrade helix-core to 1.4.3 to address CVE-2023-38647

[ShahimSharafudeen]

Description

Upgraded helix-core to version 1.4.3 to address CVE-2023-38647, as the vulnerable version (helix-core:1.0.4) was introduced as a transitive dependency through the presto-pinot-driver.

Motivation and Context

Impact

Test Plan

Contributor checklist

• Please make sure your submission complies with our contributing 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 ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

[imjalpreet]

@ShahimSharafudeen, thank you. Why don't we add this to the root pom's DependencyManagement?

[steveburnett]

Thanks for the release note! Minor formatting nit.

== RELEASE NOTES ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

[ShahimSharafudeen]

@ShahimSharafudeen, thank you. Why don't we add this to the root pom's DependencyManagement?

@imjalpreet - This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

[ShahimSharafudeen]

Thanks for the release note! Minor formatting nit.

== RELEASE NOTES ==

Security Changes
* Upgrade helix-core to 1.4.3 to address `CVE-2023-38647  <https://github.com/advisories/GHSA-jhcr-hph9-g7wm>`_.

Thanks @steveburnett for the correction. I've made the changes accordingly.

[imjalpreet]

This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

@ShahimSharafudeen IMO, I’d still suggest adding it to the root POM. This way, any future additions to other modules will be automatically covered, and it also simplifies version management across multiple modules.

[ShahimSharafudeen]

This transitive dependency is only used in the presto-pinot modules, which is why I added it directly to those modules instead of the root pom.xml

@ShahimSharafudeen IMO, I’d still suggest adding it to the root POM. This way, any future additions to other modules will be automatically covered, and it also simplifies version management across multiple modules.

@imjalpreet - Agreed with your point. I’ve moved the dependency to the root POM to ensure future modules are automatically covered and to simplify version management across the project.

[imjalpreet]

Thanks, LGTM, a small nit.

[imjalpreet]

nit: please add the version as a property

[ShahimSharafudeen]

Done

[imjalpreet]

Also, add a comment to explain why we have added this here.

[ShahimSharafudeen]

Done

[imjalpreet]

@ShahimSharafudeen, thank you, it LGTM, just a small suggestion to move this dependency just after the pinot-driver dependency here:

presto/pom.xml

Lines 2242 to 2246 in e53f403

	<dependency>
	<groupId>org.apache.pinot</groupId>
	<artifactId>presto-pinot-driver</artifactId>
	<version>${dep.pinot.version}</version>
	</dependency>

[imjalpreet]

Suggested change

	<!-- Add the helix-core dependency to the root pom.xml to upgrade the transitive helix-core version used by the Presto Pinot driver-->
	<!-- Upgrades the transitive helix-core version used by the Presto Pinot driver to address CVE-2023-38647 -->

[ShahimSharafudeen]

@imjalpreet – As per your request, I’ve updated the comment details based on your suggestion.

[imjalpreet]

Thanks, @ShahimSharafudeen

[tdcmeehan]

Is there an update to the Pinot driver which fixes this CVE?

[imjalpreet]

Is there an update to the Pinot driver which fixes this CVE?

@tdcmeehan, I had checked this earlier. There hasn't been any new version released since March 2023, and the latest version is still using helix-core:1.0.4.

https://mvnrepository.com/artifact/org.apache.pinot/presto-pinot-driver/0.12.1

[tdcmeehan]

Can we create an issue in Pinot and link to that issue here? And potentially contribute this change upstream to the Pinot driver?

[imjalpreet]

@tdcmeehan, the presto-pinot-driver was deprecated by Pinot in 2023 when they moved to a higher JDK version, but at that time we were still on JDK 8. Instead of continuing with the presto-pinot-driver, we needed to upgrade to the latest Pinot libraries, but the Java version held us back until now.

This PR was originally intended as a temporary CVE fix for the existing implementation we had added at IBM before upgrading to JDK 17.

Now that we’re on Java 17, the Pinot library upgrade is already underway. We’re moving to the latest libraries and removing the deprecated presto-pinot-driver. I checked the status, and it’s further along than I initially thought when I reviewed a couple of weeks back, so it might make sense to hold off on merging this PR and instead complete the upgrade, especially since that process already includes bringing the helix-core:1.4.3 dependency into Presto.

Pinot library upgrade PR: #25785

[ShahimSharafudeen]

Thanks, @imjalpreet , for your clarification on the Pinot driver change during my absence.

@tdcmeehan @imjalpreet — Could you please confirm whether this PR is still relevant to keep open, since the same fix is already included in the Pinot library upgrade PR?

[imjalpreet]

@ShahimSharafudeen, we can close this in favour of #25785