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

Is there a way to use tokio + rustls +webpki-roots ? #1155

Open
axelfaure opened this issue Apr 19, 2024 · 13 comments · May be fixed by #1160
Open

Is there a way to use tokio + rustls +webpki-roots ? #1155

axelfaure opened this issue Apr 19, 2024 · 13 comments · May be fixed by #1160

Comments

@axelfaure
Copy link

Hi!

I am looking for a way to use this crate with tokio and rustls (so basically tokio-rustls-comp feature) but I would like to use webpki-roots instead of rustls-native-certs. I want this specific setup because that is what I use in every other tls setup in my project and because my goal is to run my service in a very minimal environment (ideally without openssl and without local certificates).

redis-rs seem to support using webpki-roots only in a sync context but not with tokio. I do not think there is any specific reason behind that, it only seems to be an unwanted consequence of the way features are split and combined.

Could anyone please confirm that it is indeed not currently possible? And if so, do you think we could rework current features to make it possible?

@nihohit
Copy link
Contributor

nihohit commented Apr 19, 2024

@djc do you know about this?

@djc
Copy link
Contributor

djc commented Apr 19, 2024

There is a tls-rustls-webpki-roots Cargo feature, have you tried it? You just need to avoid the more general tls-rustls feature.

@axelfaure
Copy link
Author

axelfaure commented Apr 19, 2024

I did, I tried several combinations including tls-rustls-webpki-roots + tokio-comp ( with and without tokio-rustls) but I couldn't get it to compile.

tls-rustls-webpki-roots by itself does not seem to support tokio

@djc
Copy link
Contributor

djc commented Apr 22, 2024

Right, but you can combine it with tokio features. I'm pretty sure that it's possible.

@nihohit
Copy link
Contributor

nihohit commented Apr 22, 2024

Hi, Have you tried building with tls-rustls-webpki-roots + tokio-rustls-comp? This works for me.
Trying to use the features of tokio-rustls-comp without the feature itself might be why you failed.

@axelfaure
Copy link
Author

Hi !

Thanks for the answer. I re-read redis-rs Cargo.toml and I realized I misunderstood what was actually done.

I assumed tls-rustls-webpki-roots would allow me to avoid depending on rustls-native-certs entirely. That is why I tried to avoid depending on tokio-rustls-comp -> tls-rustls -> rustls-native-certs. What I didn't realize was that tls-rustls-webpki-roots itself had a dependency to tls-rustls and therefore to rustls-native-certs.

So if I am correct this time, I can use webpki-roots with tokio but by doing that, I still have a "dead" dependency to rustls-native-certs. is that right? If that is the case, I assume I could indeed use my binary in an env without local certificates.

I guess I could live with that tradeoff. Isn't it a bit unfortunate to still depends on rustls-native-certs in this situation though (in my case I was trying to blacklist this crate in my CI to avoid unwillingly introducing a dependency to these files)

@djc
Copy link
Contributor

djc commented Apr 22, 2024

No, I'm pretty sure you can make do without the "dead" dependency.

@nihohit
Copy link
Contributor

nihohit commented Apr 22, 2024

If the dependency itself is problematic, you're more than welcome to open a PR that removes this dependency when using webpki. I'm not a Cargo.toml magician, so I don't know off-the-cuff how to do that, but I believe it should be possible.

@axelfaure
Copy link
Author

axelfaure commented Apr 22, 2024

@djc if you have any tips on how to do that without enabling the tls-rustls-webpki-roots which itself seems to depend on rustls-native-certs please let me know.

@nihohit I will give it a try.
I think we could have the following features:

  • tls-rustls-core: internal feature with no dependency on any cert provider,
  • tls-rustls-webpki-roots that depends on tls-rustls-core and adds webpki-roots
  • tls-rustls-native-certs that depends on tls-rustls-core and adds rustls-native-certs
  • tls-rustls would only be kept for backward compat as an alias of tls-rustls-native-certs

I am not quite sure how these features currently interact with async-std and tokio runtimes. Ideally, I would be tempted to avoid having to define combined features such as tokio-native-tls-comp in favour of having separate

  • tokio-comp / async-std-comp ( / nothing if we don't want an async runtime)
  • native-tls / rustls-webpki-roots / rustls-native-certs

But I am not entirely sure how difficult it would be.

@djc
Copy link
Contributor

djc commented Apr 23, 2024

I agree that the feature setup isn't great here in that tls-rustls probably shouldn't enable rustls-native-certs, but it still seems that you can just enable ['rustls', 'rustls-pemfile', 'rustls-pki-types', 'tokio-comp', 'tokio-rustls', 'webpki-roots']? It seems you don't really understand Cargo features and I'm a little frustrated having to explain it here when you could read the manual or ask on a support forum instead.

@axelfaure
Copy link
Author

I may be wrong and I don't claim to be an expert on the matter but I am pretty confident I do know how Cargo features work.

I am in particular aware of the fact that features are not only used to enable optional dependencies but are also usually used in rust code to control conditional compilation.

I can indeed make redis-rs compile with the set of features you suggested and it will appear to work because it includes tokio-comp which is an actual feature that will allow me to use redis in an async context.

However, regarding rustls, you only enable dep-features that are not used as switch in rust code for conditional compilation. The result would be that all rustls related crate you included will be downloaded by cargo but they won't actually be used by redis-rs since we wont trigger any of the compilation conditions in rust code such as #[cfg(feature = "tls-rustls")] or #[cfg(any(feature = "tokio-native-tls-comp", feature = "tokio-rustls-comp"))]

This is made obvious if you run it through clippy while enabling unused_crate_dependencies warning :

$ cargo clippy -F rustls -F rustls-pemfile -F rustls-pki-types -F tokio-comp -F tokio-rustls -F webpki-roots -- -W unused_crate_dependencies
    Checking redis v0.25.3 (/home/afe/Dev/redis-rs/redis)
warning: external crate `rustls` unused in `redis`: remove the dependency or add `use rustls as _;`
  |
  = note: requested on the command line with `-W unused-crate-dependencies`

warning: external crate `rustls_pemfile` unused in `redis`: remove the dependency or add `use rustls_pemfile as _;`

warning: external crate `rustls_pki_types` unused in `redis`: remove the dependency or add `use rustls_pki_types as _;`

warning: external crate `tokio_rustls` unused in `redis`: remove the dependency or add `use tokio_rustls as _;`

warning: external crate `webpki_roots` unused in `redis`: remove the dependency or add `use webpki_roots as _;`

warning: `redis` (lib) generated 5 warnings

@djc
Copy link
Contributor

djc commented Apr 23, 2024

If you know how Cargo features work, then I'm surprised you could not come up with the set of features I listed yourself.

It does indeed seem that the feature setup here is broken.

@axelfaure
Copy link
Author

@nihohit I created a PR as you suggested.
I couldn't decorrelate the async framework from the tls backend without creating way more unused dependencies. So I went the other way and tried to have only one needed feature per use-case.
There are certainly other ways to achieve the same result so let me know if you think that is not the way you would like to implement it.

By the way, I think rustls_pemfile is not always needed (it is only required by functions that are dead code if we don't enable additional features). But this issue was more tricky to tackle and I didn't want to risk breaking another set of features by modifying it in the same PR.

barshaul pushed a commit to barshaul/redis-rs that referenced this issue Jul 11, 2024
* Java: Add `Time()` command (redis-rs#145)

* Add Time() command to Java client

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Javadoc update

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Clean javadoc

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Remove warning in unit test for time()

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants