Skip to content

Upgrade grpcio to 1.60.0 to fix CVE-2023-1428 #174

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Upgrade grpcio to 1.60.0 to fix CVE-2023-1428 #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63c7206
Upgrade grpcio to 1.60.0 to fix CVE-2023-1428
BewareMyPower Dec 12, 2023
16f1228
Add a workflow to verify functions installation
BewareMyPower Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci-pr-validation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ jobs:
- name: Stop Pulsar service
run: ./build-support/pulsar-test-service-stop.sh

- name: Test functions install
run: |
WHEEL=$(find dist -name '*.whl')
pip3 install ${WHEEL}[functions] --force-reinstall

linux-wheel:
name: Wheel ${{matrix.image.name}} - Py ${{matrix.python.version}} - ${{matrix.cpu.platform}}
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def build_extension(self, ext):
extras_require["functions"] = sorted(
{
"protobuf>=3.6.1,<=3.20.3",
"grpcio>=1.8.2",
"grpcio>=1.60.0",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"grpcio>=1.60.0",
"grpcio>=1.53.0",

Why not use 1.53.0, the minimum viable version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, when upgrading a dependency, it's better to use the latest version at that moment if it does not break the compatibility. Though this repo does not check the compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

This is the dependency requirement instead of the specific dependency version. It's not like the dependency version control like in maven pom. I think maybe a good practice is to upgrade the requirement to the minimum viable version. What do you think of this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's compatible to install, the version will be minimum required version. I tested it locally and verified 1.60.0 could be installed with other dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a workflow to verify the installation in CI. It's not like Maven because sometimes the Python wheel uses the underlying C extension (like the Pulsar Python C++ client) and a specific version of dependency A might not be compatible with dependency B due to the symbols conflict or something else.

However, if all dependencies are compatible, using the latest version is good. Here we use grpcio>=1.60.0 means in future if another dependency breaks the compatibility with grpc==1.60.0, there will be a chance to install a higher version of grpcio.

Copy link
Member

Choose a reason for hiding this comment

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

What if users introduce another dependency compatible with grpcio 1.53.0 but not with 1.60? Maybe that dependency depends on a behavior supported in 1.53.0 but not 1.60.0? When installing the Python client, the pip will upgrade the existing grpcio to 1.60.0, which may conflict with other dependencies.

I didn't find any breaking changes from grpcio 1.53.0 to 1.60.0. So I'm OK with this PR. However, I would like to discuss the best practice for the dependency requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if users introduce another dependency compatible with grpcio 1.53.0 but not with 1.60?

Another question: what if users introduced another dependency compatible with grpcio 1.60 but not with 1.53.0?

There is no best practice. But in general, a new release should be better than an older one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When installing the Python client, the pip will upgrade the existing grpcio to 1.60.0, which may conflict with other dependencies.

However, you assume the existing grpcio version is in [1.53, 1.60). If the grpcio version is less than 1.53, there will be no difference.

Copy link
Member

Choose a reason for hiding this comment

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

Another question: what if users introduced another dependency compatible with grpcio 1.60 but not with 1.53.0?

That should be a common case. Support we use grpcio>=1.53.0, then that another dependency should define the requirements with grpcio>=1.60.0. The pip will upgrade the grpcio to 1.60, and everything should be fine.

However, you assume the existing grpcio version is in [1.53, 1.60). If the grpcio version is less than 1.53, there will be no difference.

Why? I am assuming the grpcio version <1.60.

Suppose we have another dependency A that requires grpcio version in [1.40,1.55].
If the existing grpcio version < 1.53(Let's suppose it as 1.40) :

  1. If we use grpcio>=1.53.0, the pip will upgrade the grpcio to 1.53.0. All works fine.
  2. If we use grpcio>=1.60.0, the pip will uggrade the grpcio to 1.60.0. The dependency A isn't compatible with this.

"apache-bookkeeper-client>=4.16.1",
"prometheus_client",
"ratelimit"
Expand Down