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

fix(reqwest): add default-tls feature to reqwest #75

Closed
wants to merge 2 commits into from

Conversation

hydezhao
Copy link

@hydezhao hydezhao commented Jul 19, 2024

enable default-tls feature to reqwest so that the binary can use OS's CA keystore.

It solves the problem that we cannot download cast from a self-hosted server with a private CA certificate.

@ku1ik
Copy link
Contributor

ku1ik commented Oct 5, 2024

Thanks.

How does reqwest handle certs when both default-tls and rustls-tls are enabled? Is the behavior documented somewhere?

@hydezhao
Copy link
Author

hydezhao commented Oct 7, 2024

Hello, from what I learnt, cargo features are additive..

@ku1ik
Copy link
Contributor

ku1ik commented Oct 15, 2024

Cargo features are additive, yeah, I'm just not sure which variant reqwest uses when multiple are enabled. I'm pretty sure one always wins over the other, I doubt it both would be active (e.g. one being fallback to the other).

My concern here is default-tls uses native TLS (OpenSSL), which makes the build process more complicated and fragile (need for OpenSSL headers, requires compiling C code, etc). I've actually tried using it initially, for the first version of agg, and I couldn't make it build on some platform/environment (don't remember which).

What about rustls-tls-native-roots? I think this could be the best of both worlds, using pure Rust rustls crate, but looking up certificates in common OS locations.

Can you test if it works with your private CA cert when you replace rustls-tls feature with rustls-tls-native-roots (and not enable default-tls)?

@ku1ik
Copy link
Contributor

ku1ik commented Oct 15, 2024

I actually just made this change in main branch, so you can just checkout main and test against your cert.

@hydezhao
Copy link
Author

Hello @ku1ik , thanks a lot for your reply.
I got some local issue with my local build of agg . ( Error: application/octet-stream is not supported ) I will try figure it out .

Meanwhile , I've tested rustls-tls-native-roots on project asciinema. With rustls-tls-native-roots I can successfully upload record to my asciinema server with private CA cert. I assume that it works !

Thanks a lot !

@hydezhao
Copy link
Author

I've updated the PR asciinema/asciinema#643 accordingly .

@hydezhao
Copy link
Author

I think we can close this PR.

@hydezhao hydezhao closed this Oct 16, 2024
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 this pull request may close these issues.

2 participants