-
Notifications
You must be signed in to change notification settings - Fork 94
Switch to rustls #3811
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.
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
base: main
Are you sure you want to change the base?
Switch to rustls #3811
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Replace native-tls (OpenSSL) with rustls-tls in the workspace reqwest dependency to enable cross-compilation to musl targets without requiring OpenSSL cross-compilation toolchain. This change adds `rustls-tls` and `rustls-tls-webpki-roots` features to the reqwest workspace dependency, which propagates to all crates that use reqwest via the workspace, including google-cloud-gax-internal. Benefits: - Eliminates OpenSSL dependency and cross-compilation complexity - Enables musl target builds without vendored OpenSSL or cargo-zigbuild - Reduces binary size (rustls is pure Rust, no C dependencies) - Improves musl compatibility and static linking support The change maintains API compatibility as reqwest's rustls backend provides equivalent functionality to native-tls for HTTP client operations.
689e30d to
b43bd68
Compare
… compatibility Replace tls-native-roots with tls-webpki-roots in tonic dependency to ensure pure Rust certificate root loading without system dependencies. This improves musl compatibility and eliminates potential OpenSSL dependencies for certificate loading.
coryan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the right fix. See #1533
| prost-build = { default-features = false, version = "0.14" } | ||
| prost-types = { default-features = false, version = "0.14" } | ||
| rand = { default-features = false, version = "0.9.2" } | ||
| reqwest = { default-features = false, version = "0.12.24", features = ["json"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... my understanding is that you can enable these features in your own Cargo.toml file, and if we enable them here no downstream dependency could disable them. Is that not the case?
| time = { default-features = false, version = "0.3.44" } | ||
| tokio = { default-features = false, version = "1" } | ||
| tokio-metrics = { default-features = false, version = "0.4.5" } | ||
| tonic = { default-features = false, version = "0.14.1", features = ["tls-native-roots", "tls-ring"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a problem. But we should fix it by reducing the number of default features.
|
I should have mentioned. If you think I am wrong just say so. I am sure our handling of TLS dependencies is far from perfect. |
Replace native-tls (OpenSSL) with rustls-tls in the workspace reqwest
dependency to enable cross-compilation to musl targets without requiring
OpenSSL cross-compilation toolchain.
This change adds
rustls-tlsandrustls-tls-webpki-rootsfeatures tothe reqwest workspace dependency, which propagates to all crates that
use reqwest via the workspace, including google-cloud-gax-internal.
Benefits:
The change maintains API compatibility as reqwest's rustls backend
provides equivalent functionality to native-tls for HTTP client operations.