Skip to content
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

Feature request [rust]: allow specifying TLS flavour to use #889

Closed
msdinit opened this issue Apr 13, 2023 · 6 comments · Fixed by #892
Closed

Feature request [rust]: allow specifying TLS flavour to use #889

msdinit opened this issue Apr 13, 2023 · 6 comments · Fixed by #892
Labels
lib/rust Rust client library

Comments

@msdinit
Copy link

msdinit commented Apr 13, 2023

Feature Request

Motivation

We are building our application against musl, cross-compiling it from regular libc Ubuntu. However, we are having issues with svix client, because it depends on system OpenSSL, which in our case would be linked against libc. There are workarounds, but they are a bit involved.

Proposal

Allow user to choose whether openssl or rustls version of reqwest will be used by the svix client

Alternatives

Alternative is to either vendor openssl, comiling it each time, or to use a pre-made Docker image with openssl compiled against musl included. However, we are already using reqwest with rustls, and would like to avoid having both flavours in the dependency tree, as well as extra complexity in the build pipeline.

@tasn
Copy link
Member

tasn commented Apr 13, 2023

Thanks @msdinit!

We actually explicitly switched away from rustls to openssl because it was causing issues. Take a look at #704, and let me know your thoughts.

Use openssl instead of rusttls for outbound webhooks. In our experience,
many widely deployed webservers use "weak" ciphers that rusttls refuses to
support, leaving us with no choice but to rely on openssl libraries.

@msdinit
Copy link
Author

msdinit commented Apr 13, 2023

Ah, I see. Did not notice that as I only searched through the issues, not PRs :)

Regardless though, the PR you linked deals with the server, my request is about the client.

Would you be open to having rustls as a non-default option for the client only? I believe the overhead should be minimal (rename dependencies as reqwest-openssl and reqwest-rustls and conditionally importing from them, though would need to actually try it out)

@tasn
Copy link
Member

tasn commented Apr 13, 2023

Oh, sorry, I misread your request.

100%, we can easily change that, and potentially even make rustls the default. I didn't realize we were using openssl for that. Yeah, should be trivial.

@tasn tasn added the lib/rust Rust client library label Apr 13, 2023
@msdinit
Copy link
Author

msdinit commented Apr 13, 2023

Awesome, thank you!

@marcoslopes
Copy link
Contributor

marcoslopes commented Apr 14, 2023

The following patch seems that it does what is necessary, I have then ran three commands making sure the dependencies did or did not appear on the dependency tree as expected:

 # we have only the rustls-tls
 cargo tree --features rustls-tls --no-default-features

 # we have both since features are additive
 cargo tree --features rustls-tls

 # we have only default-tls
 cargo tree 
commit a212a162747e79cc6e814caa29764c15f9b0c65f
Author: Marcos <m...com>
Date:   Fri Apr 14 10:15:50 2023 +1000

    Feature request [rust]: allow specifying TLS flavour to use

    towards issue https://github.com/svix/svix-webhooks/issues/889

diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index a083cdd..bb118eb 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -25,5 +25,9 @@ serde = "^1.0"
 serde_derive = "^1.0"
 serde_json = "^1.0"
 url = "^2.2"
-reqwest = { version = "^0.11", features = ["json", "multipart", "default-tls"], default-features = false }
+reqwest = { version = "^0.11", features = ["json", "multipart"], default-features = false }
+
+[features]
+default = ["reqwest/default-tls"]
+rustls-tls = ["reqwest/rustls-tls"]

@tasn
Copy link
Member

tasn commented Apr 14, 2023

Oh awesome @marcoslopes, mind opening a PR? We would love to merge it.

marcoslopes added a commit to marcoslopes/svix-webhooks that referenced this issue Apr 15, 2023
// only the default-tls
cargo tree

// both since features are additive
cargo tree --features rustls-tls

// only rustls-tls
cargo tree --features rustls-tls --no-default-features

closes svix#889
marcoslopes added a commit to marcoslopes/svix-webhooks that referenced this issue Apr 15, 2023
// only the default-tls
cargo tree

// both since features are additive
cargo tree --features rustls-tls

// only rustls-tls
cargo tree --features rustls-tls --no-default-features

closes svix#889
svix-daniel pushed a commit that referenced this issue Apr 18, 2023
// only the default-tls
cargo tree

// both since features are additive
cargo tree --features rustls-tls

// only rustls-tls
cargo tree --features rustls-tls --no-default-features

closes #889

## Motivation

Bellow is verbatim copy from issue #889 created by @msdinit

We are building our application against musl, cross-compiling it from
regular libc Ubuntu. However, we are having issues with svix client,
because it depends on system OpenSSL, which in our case would be linked
against libc. There sfackler/rust-openssl#603,
but they are a bit involved.

## Solution

Added cargo features allowing users to choose which ssl version to use
for svix reqwest http client, keeping backwards compatibility.
svix-gabriel pushed a commit that referenced this issue Apr 18, 2023
// only the default-tls
cargo tree

// both since features are additive
cargo tree --features rustls-tls

// only rustls-tls
cargo tree --features rustls-tls --no-default-features

closes #889

## Motivation

Bellow is verbatim copy from issue #889 created by @msdinit

We are building our application against musl, cross-compiling it from
regular libc Ubuntu. However, we are having issues with svix client,
because it depends on system OpenSSL, which in our case would be linked
against libc. There sfackler/rust-openssl#603,
but they are a bit involved.

## Solution

Added cargo features allowing users to choose which ssl version to use
for svix reqwest http client, keeping backwards compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib/rust Rust client library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants