You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "tomncooper (via GitHub)" <gi...@apache.org> on 2024/11/01 16:12:31 UTC
[PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
tomncooper opened a new pull request, #25602:
URL: https://github.com/apache/flink/pull/25602
## What is the purpose of the change
There is a high severity CVE ([CVE-2023-1370](https://nvd.nist.gov/vuln/detail/CVE-2023-1370)) in the json-path version used by Calcite 1.32 used in the `flink-table-calcite-bridge` module.
Newer versions of Calcite update to newer versions of `json-path`. However, updating Calcite to the latest version ([FLINK-36602](https://issues.apache.org/jira/browse/FLINK-36602)) is not straightforward and involves changes to the SQL parsing logic. Following [discussion](https://lists.apache.org/thread/7ogwvj5z3o176dw95145dzvlolrkyps4) on the dev mailing list, an incremental Calcite upgrade process is preferred. Therefore, this PR simply patches the transitive dependency.
## Brief change log
This PR overrides the specific transitive `json-path` (version 2.7.0) dependency in the `flink-table-calcite-bridge` pom file to version 2.9.0.
## Verifying this change
This change is already covered by existing tests in the `flink-table` module.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): yes
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "tomncooper (via GitHub)" <gi...@apache.org>.
tomncooper commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2454479539
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33571][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25602:
URL: https://github.com/apache/flink/pull/25602#discussion_r1864931775
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -152,9 +152,21 @@ under the License.
<groupId>org.locationtech.proj4j</groupId>
<artifactId>proj4j</artifactId>
</exclusion>
+ <!-- Exclude json-path as we are manually overriding it to a newer version -->
+ <exclusion>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ </exclusion>
</exclusions>
</dependency>
+ <!-- Override the json-path version used by Calcite 1.32 to deal with CVE-2023-1370 -->
Review Comment:
I don't think it is good practice to say that we override something in one of the dependencies and mentioning its version.
Problem 1: now it is not 1.32, and as a result it will only confuse people since not clear why we are talking about 1.32 here or whatever other version if it doesn't match the actual dependency
Problem 2: It says nothing what to do with it. In fact I would replace this comment with something like
```
TODO: remove together with Calcite upgrade to 1.38.0, more details are at FLINK-XXXX (put your jira issue here)
```
Then it would be much clear when and what we should do with this and where to go to get more info
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36830][table] Override json-path version in Calcite to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #25602:
URL: https://github.com/apache/flink/pull/25602
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33571][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25602:
URL: https://github.com/apache/flink/pull/25602#discussion_r1864931775
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -152,9 +152,21 @@ under the License.
<groupId>org.locationtech.proj4j</groupId>
<artifactId>proj4j</artifactId>
</exclusion>
+ <!-- Exclude json-path as we are manually overriding it to a newer version -->
+ <exclusion>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ </exclusion>
</exclusions>
</dependency>
+ <!-- Override the json-path version used by Calcite 1.32 to deal with CVE-2023-1370 -->
Review Comment:
I don't think it is good practice to say that we override something in one of the dependencies and mentioning its version.
Problem 1: now it is not 1.32, and as a result it will only confuse people since not clear why we are talking about 1.32 here or whatever other version if it doesn't match the actual dependency.
Problem 2: It says nothing what to do with it. In fact I would replace this comment with something like
```
TODO: remove together with Calcite upgrade to 1.38.0, more details are at FLINK-XXXX (put your jira issue here)
```
Then it would be much clear when and what we should do with this and where to go to get more info
UPD: Problem 3: there is also 1.32 both in commit message and PR name
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -152,9 +152,21 @@ under the License.
<groupId>org.locationtech.proj4j</groupId>
<artifactId>proj4j</artifactId>
</exclusion>
+ <!-- Exclude json-path as we are manually overriding it to a newer version -->
+ <exclusion>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ </exclusion>
</exclusions>
</dependency>
+ <!-- Override the json-path version used by Calcite 1.32 to deal with CVE-2023-1370 -->
Review Comment:
I don't think it is good practice to say that we override something in one of the dependencies and mentioning its version.
Problem 1: now it is not 1.32, and as a result it will only confuse people since not clear why we are talking about 1.32 here or whatever other version if it doesn't match the actual dependency.
Problem 2: It says nothing what to do with it. In fact I would replace this comment with something like
```
TODO: remove together with Calcite upgrade to 1.38.0, more details are at FLINK-XXXX (put your jira issue here)
```
Then it would be much clear when and what we should do with this and where to go to get more info
UPD:
Problem 3: there is also 1.32 both in commit message and PR name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2452155281
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "fdde6c12c906f60845397764704e514c1c35b9d7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "fdde6c12c906f60845397764704e514c1c35b9d7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* fdde6c12c906f60845397764704e514c1c35b9d7 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33571][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2506290502
It looks like we've pivoted from the original intent of the Jira ticket (upgrade to Calcite 1.38). That means that we'll either have to create a new Jira ticket for what's done in this PR, or we have to update the existing Jira ticket to reflect the changes from this PR and create a new one to deal with the upgrade to Calcite 1.38
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2501982077
btw referencing to FLINK-36602 here is not a right way since the PR doesn't solve the issue (which is called `Upgrade Calcite version to 1.38.0`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33571][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25602:
URL: https://github.com/apache/flink/pull/25602#discussion_r1864931775
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -152,9 +152,21 @@ under the License.
<groupId>org.locationtech.proj4j</groupId>
<artifactId>proj4j</artifactId>
</exclusion>
+ <!-- Exclude json-path as we are manually overriding it to a newer version -->
+ <exclusion>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ </exclusion>
</exclusions>
</dependency>
+ <!-- Override the json-path version used by Calcite 1.32 to deal with CVE-2023-1370 -->
Review Comment:
I don't think it is good practice to say that we override something in one of the dependency and mentioning its version.
Problem 1: now it is not 1.32, and as a result it will only confuse people since not clear why we talking about 1.32 here or whatever other version if it doesn't match the actual dependency
Problem2: It says nothing what to do with it. In fact I would replace this comment with something like
```
TODO: remove together with Calcite upgrade to 1.38.0, more details are at FLINK-XXXX (put your jira issue here)
```
Then it would be much clear when and what we should do with this and where to go to get more info
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36830][table] Override json-path version in Calcite to deal with CVE [flink]
Posted by "tomncooper (via GitHub)" <gi...@apache.org>.
tomncooper commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2511372760
@MartijnVisser I have create a separate JIRA issue ([FLINK-36830](https://issues.apache.org/jira/browse/FLINK-36830)) for this PR which links out to the others discussed here. If you want me to add any more context to that, let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2496183127
it looks like it doesn't solve the issue (just execute `mvnw dependency:tree` to check it)
seems first need to be fixed in flink-shaded and then here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33571][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "tomncooper (via GitHub)" <gi...@apache.org>.
tomncooper commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2503426742
@snuyanzin Good point, I ammended the title and commit message to point at [FLINK-33571](https://issues.apache.org/jira/browse/FLINK-33571).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36602][table] Override json-path version for Calcite 1.32 to deal with CVE [flink]
Posted by "tomncooper (via GitHub)" <gi...@apache.org>.
tomncooper commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2497741017
@snuyanzin you are right this won't fix the direct `json-path` dependency until Flink-Shaded is updated. I opened a [PR](https://github.com/apache/flink-shaded/pull/140) for that in flink-shaded and we will need to move the direct `json-path` dep once flink-shaded does a new release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36830][table] Override json-path version in Calcite to deal with CVE [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25602:
URL: https://github.com/apache/flink/pull/25602#issuecomment-2524256729
a note: this PR doesn't solve CVE issue.
Too solve it completely we also need to release flink-shaded (with updated json-path dependency) and bump it's version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36830][table] Override json-path version in Calcite to deal with CVE [flink]
Posted by "tomncooper (via GitHub)" <gi...@apache.org>.
tomncooper commented on code in PR #25602:
URL: https://github.com/apache/flink/pull/25602#discussion_r1865741269
##########
flink-table/flink-table-calcite-bridge/pom.xml:
##########
@@ -152,9 +152,21 @@ under the License.
<groupId>org.locationtech.proj4j</groupId>
<artifactId>proj4j</artifactId>
</exclusion>
+ <!-- Exclude json-path as we are manually overriding it to a newer version -->
+ <exclusion>
+ <groupId>com.jayway.jsonpath</groupId>
+ <artifactId>json-path</artifactId>
+ </exclusion>
</exclusions>
</dependency>
+ <!-- Override the json-path version used by Calcite 1.32 to deal with CVE-2023-1370 -->
Review Comment:
All good points, I have updated the comments and git commit message to remove versions and added JIRA references for more information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org