fix(security): Upgrade pinot to version 1.40 and Override vulnerable lz4-java dependency#26684
Conversation
prestodb-ci
requested review from
a team,
NivinCS and
pramodsatya
and removed request for
a team
presto-pinot-toolkit/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.glassfish.jersey.core</groupId> | ||
| <artifactId>jersey-client</artifactId> | ||
| <version>2.47</version> |
There was a problem hiding this comment.
@imsayari404 , Is there any specific reason why we aren’t considering upgrading to the latest available version of jersey-client from the Maven repository?
There was a problem hiding this comment.
Thank you for your review,
before my change :
presto-pinot-toolkit:jar:0.296-SNAPSHOT
\- org.apache.pinot:pinot-common:jar:1.3.0:compile
\- org.glassfish.jersey.core:jersey-server:jar:2.45:compile
\- org.glassfish.jersey.core:jersey-client:jar:2.45:compile
after my change :
presto-pinot-toolkit:jar:0.296-SNAPSHOT
\- org.glassfish.jersey.core:jersey-client:jar:2.47:compile
The presto-pinot-toolkit module specifically pulls in jersey-client 2.45 through the pinot-common dependency.
Pinot 1.3.0 uses Jersey 2.x series. I made the change because I thought staying within the 2.x line will maintain better compatibility with the Pinot dependency.
That said, I'm open to upgrading to 4.x
There was a problem hiding this comment.
I think it would be better to upgrade to the latest version, as long as there are no compatibility issues
presto-pinot-toolkit/pom.xml
Outdated
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.glassfish.jersey.core</groupId> | ||
| <artifactId>jersey-client</artifactId> |
There was a problem hiding this comment.
Why are we excluding jersey-client in this context?
There was a problem hiding this comment.
The pinot-common:1.3.0 dependency transitively brings in jersey-client:2.45, which contains the security vulnerability (CVE-2025-12383).
By excluding the transitive dependency and declaring jersey-client:2.47 explicitly in our POM, we ensure that the secure version is used instead of the vulnerable one.
There was a problem hiding this comment.
Thanks for the clarification. If this vulnerability is coming transitively from pinot-common:1.3.0, should we consider upgrading pinot-common to 1.4.0? This vulnerability is not listed for version 1.4.0 in the Maven repository. If upgrading pinot-common resolves the issue, then we may not need to upgrade jersey-client explicitly
presto-pinot-toolkit/pom.xml
Outdated
| </goals> | ||
| <configuration> | ||
| <ignoredUnusedDeclaredDependencies> | ||
| <ignoredUnusedDeclaredDependency>org.glassfish.jersey.core:jersey-client</ignoredUnusedDeclaredDependency> |
There was a problem hiding this comment.
Why are we adding org.glassfish.jersey.core:jersey-client to ignoredUnusedDeclaredDependency?
There was a problem hiding this comment.
While presto-pinot-toolkit doesn't directly reference jersey-client classes in its own code, it's consumed transitively through pinot-common. Since we're explicitly managing this dependency version, the plugin will need to ignore this "unused" warning.
There was a problem hiding this comment.
The above comment applies here as well.
imjalpreet
left a comment
imjalpreet
left a comment
There was a problem hiding this comment.
Thank you, @imsayari404.
We should avoid upgrading transitive dependencies when possible. In this case, the Pinot 1.4.0 release already updates the Jersey dependencies to 2.47 (https://github.com/apache/pinot/blob/3a56c9db545e3b403863d8319055ea9be8eae771/pom.xml#L150), so we should try to upgrade Pinot from 1.3.0 to 1.4.0 and assess the amount of work required.
imsayari404
force-pushed
the
jersey_client_cve
branch
from
5198922 to
67888e0
Compare
imsayari404
changed the title
|
I've pushed some changes related to the upgrade work, but I'm currently facing an issue while testing the Pinot connector locally on Presto OSS master. I'm looking into this at the moment and will update once I identify the root cause. This pr will resolve this issue : feat(plugin-pinot): Add TLS support #26151 (not yet merged) |
imsayari404
force-pushed
the
jersey_client_cve
branch
3 times, most recently
from
2fe8dd9 to
5189d6b
Compare
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpgrades Pinot from 1.3.0 to 1.4.0 to address a Jersey client CVE and adjusts the Pinot connector’s gRPC client, request builder usage, and tests to match the new Pinot 1.4.0 APIs and DataTable semantics. Class diagram for updated Pinot gRPC client and request builder usageclassDiagram
class PinotStreamingQueryClient {
- Map~String, ServerGrpcQueryClient~ grpcQueryClientMap
- GrpcConfig config
+ PinotStreamingQueryClient(GrpcConfig config)
+ Iterator~Server_ServerResponse~ submit(String host, int port, ServerGrpcRequestBuilder requestBuilder)
- ServerGrpcQueryClient getOrCreateGrpcQueryClient(String host, int port)
}
class GrpcConfig {
}
class ServerGrpcQueryClient {
+ ServerGrpcQueryClient(String host, int port, GrpcConfig config)
+ Iterator~Server_ServerResponse~ submit(Object request)
}
class ServerGrpcRequestBuilder {
+ Object build()
}
class PinotProxyGrpcRequestBuilder {
}
PinotStreamingQueryClient --> GrpcConfig : has
PinotStreamingQueryClient --> ServerGrpcQueryClient : caches
PinotStreamingQueryClient --> ServerGrpcRequestBuilder : uses
PinotProxyGrpcRequestBuilder --|> ServerGrpcRequestBuilder : extends
ServerGrpcRequestBuilder ..> ServerGrpcQueryClient : builds_requests_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
testPinotGrpcRequestassertion ongrpcRequest.getMetadataCount()being exactly 6 is likely to be brittle with future Pinot changes; consider asserting the presence and values of the specific metadata keys you care about instead of the total count.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `testPinotGrpcRequest` assertion on `grpcRequest.getMetadataCount()` being exactly 6 is likely to be brittle with future Pinot changes; consider asserting the presence and values of the specific metadata keys you care about instead of the total count.
## Individual Comments
### Comment 1
<location> `presto-pinot-toolkit/src/test/java/com/facebook/presto/pinot/TestPinotSegmentPageSource.java:436` </location>
<code_context>
Assert.assertEquals(grpcRequest.getSegmentsCount(), 1);
Assert.assertEquals(grpcRequest.getSegments(0), "segment1");
- Assert.assertEquals(grpcRequest.getMetadataCount(), 5);
+ Assert.assertEquals(grpcRequest.getMetadataCount(), 6);
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.REQUEST_ID), "121");
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.BROKER_ID), "presto-coordinator-grpc");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the new metadata key/value instead of only the metadata count
Increasing the expected metadata count confirms that an extra entry exists, but not that the new Pinot 1.4.0 metadata is present or correct. To make the test more robust, also assert the specific key and value for the new entry (e.g., `assertEquals(grpcRequest.getMetadataOrThrow(<NEW_KEY>), <expectedValue>)`) so the test fails if the wrong metadata is set or the new entry is removed.
Suggested implementation:
```java
Assert.assertEquals(grpcRequest.getSegmentsCount(), 1);
Assert.assertEquals(grpcRequest.getSegments(0), "segment1");
Assert.assertEquals(grpcRequest.getMetadataCount(), 6);
// New Pinot 1.4.0 metadata: verify the ENABLE_STREAMING flag is present and correct
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.ENABLE_STREAMING), "true");
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.REQUEST_ID), "121");
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.BROKER_ID), "presto-coordinator-grpc");
Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.ENABLE_TRACE), "false");
```
If the actual metadata key name introduced by Pinot 1.4.0 differs from `ENABLE_STREAMING` (e.g., it uses a different constant or resides under a different enum/class), adjust `CommonConstants.Query.Request.MetadataKeys.ENABLE_STREAMING` to the correct constant. Also ensure that the `ServerGrpcRequestBuilder#setEnableStreaming(true)` call is indeed what triggers this metadata entry; if the new metadata comes from a different setting, point the assertion to the appropriate key and expected value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
imsayari404
force-pushed
the
jersey_client_cve
branch
2 times, most recently
from
e143225 to
e66f892
Compare
imsayari404
force-pushed
the
jersey_client_cve
branch
from
e66f892 to
a014838
Compare
imjalpreet
left a comment
imjalpreet
left a comment
There was a problem hiding this comment.
As discussed offline, let's verify with a local Pinot deployment without SSL enabled.
|
Thanks for the release note! A nit of formatting: |
|
@imjalpreet imported this issue as lakehouse/presto #26684 |
imsayari404
force-pushed
the
jersey_client_cve
branch
from
a014838 to
53360b7
Compare
|
@imjalpreet My approach: I set up a pinot cluster in my local and tested scenarios :
So, I'll be removing bytes from unsupported types |
imsayari404
force-pushed
the
jersey_client_cve
branch
from
336a5dc to
ba7f652
Compare
|
|
||
| private static final Set<DataSchema.ColumnDataType> UNSUPPORTED_TYPES = ImmutableSet.of( | ||
| OBJECT, BYTES, MAP, UNKNOWN); | ||
| OBJECT, MAP, UNKNOWN); |
There was a problem hiding this comment.
Is this change related to the upgrade for CVEs?
There was a problem hiding this comment.
No, this change is not related to the CVE upgrade. This change removes BYTES from UNSUPPORTED_TYPES because #26684 (comment)
There was a problem hiding this comment.
should we keep BYTES data type related changes in different PR?
There was a problem hiding this comment.
Sure, I raised a pr #26985.
I will remove the change from this pr.
presto-pinot-toolkit/src/test/java/com/facebook/presto/pinot/TestPinotSegmentPageSource.java
Show resolved
Hide resolved
presto-pinot-toolkit/src/test/java/com/facebook/presto/pinot/TestPinotSegmentPageSource.java
Show resolved
Hide resolved
presto-pinot-toolkit/src/test/java/com/facebook/presto/pinot/TestPinotSegmentPageSource.java
Show resolved
Hide resolved
imsayari404
force-pushed
the
jersey_client_cve
branch
2 times, most recently
from
dd05ecb to
77053dd
Compare
imsayari404
force-pushed
the
jersey_client_cve
branch
2 times, most recently
from
53d7431 to
3af2fb0
Compare
imjalpreet
left a comment
imjalpreet
left a comment
There was a problem hiding this comment.
Thanks, @imsayari404. I have a few comments.
| Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.ENABLE_TRACE), "false"); | ||
| Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.ENABLE_STREAMING), "true"); | ||
| Assert.assertEquals(grpcRequest.getMetadataOrThrow(CommonConstants.Query.Request.MetadataKeys.PAYLOAD_TYPE), "sql"); | ||
| Assert.assertEquals(grpcRequest.getMetadataOrThrow("correlationId"), "121"); |
There was a problem hiding this comment.
nit: Is correlationId not defined as a constant in org.apache.pinot.spi.utils.CommonConstants like the other metadata fields?
There was a problem hiding this comment.
yes, updated 👍🏻
presto-pinot-toolkit/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.apache.helix</groupId> | ||
| <artifactId>helix-core</artifactId> | ||
| <version>1.4.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
Why do we need this? I don't think any Pinot dependency brings this in anymore.
There was a problem hiding this comment.
reverted the change
presto-pinot/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.apache.helix</groupId> | ||
| <artifactId>helix-core</artifactId> | ||
| <version>1.4.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
reverted the change
imsayari404
force-pushed
the
jersey_client_cve
branch
from
3af2fb0 to
f38afad
Compare
…GHSA-7p63-w6x9-6gr7) fix(tests): Update metadata count assertions in PinotSegmentPageSource tests test
imsayari404
force-pushed
the
jersey_client_cve
branch
from
f38afad to
a890c69
Compare
imjalpreet
left a comment
imjalpreet
left a comment
There was a problem hiding this comment.
Thanks, @imsayari404. Changes LGTM.
I see we are upgrading Pinot as well as lz4-java in this PR, we have two options: either raise a separate PR to upgrade lz4-java or update this PR's title to reflect that as well.
imsayari404
changed the title
Thank you @imjalpreet |
|
@imsayari404, since the title might be too long, let's mention just the version upgrades there and add the detailed information about CVE number in the PR description. Also, please update the release note, it looks unrelated. |
imsayari404
changed the title
Sure, I’ve made the requested changes. Please take a look and let me know if everything looks correct @imjalpreet |
hantangwangd
left a comment
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @imsayari404, lgtm!
|
Thanks for the release note! Nit of formatting: |
7 participants
Description
Pinot 1.4.0 refactored the gRPC interface classes, renaming and relocating them.
Specific Issues faced :
GrpcQueryClient → ServerGrpcQueryClient
GrpcRequestBuilder → ServerGrpcRequestBuilder
Classes moved but stayed in the same package: org.apache.pinot.common.utils.grpc
The DataTableBuilder.setColumn() method signature changed in Pinot 1.4.0.
Affected : https://mvnrepository.com/artifact/org.glassfish.jersey.core/jersey-client/2.45 (CVE-2025-12383)
Motivation and Context
Impact
Test Plan
Tested in internal branch with this commit cherry-picked:
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.