Replace OpenSSL with rustls #1928
Conversation
cb636f5 to
d645040
Compare
|
@mre If we see that this PR works in CI and on different machines for users, do you agree that it makes sense to fully get rid of the OpenSSL approach? Or do you think we should keep it and only change the default? |
d645040 to
fe1e69a
Compare
|
TBH, for now I would keep it and change the default. At least for one version. Then we can tell people to switch back to OpenSSL if there are any problems. We could mention that in the release-notes. On the other side, I'm flexible here. That's just what I would do, but we can also go all-in on rustls. 😆 Worst case, we release a patch version with the OpenSSL option available again. So whatever you believe is the best tradeoff between simplicity and user experience. |
|
This unfortunately is blocked by reacherhq/check-if-email-exists#1625. The problem is that the latest version of check-if-email-exists on crates.io uses openSSL without an option to use ruslts. |
|
I just found that the But it's enabled by default for lychee CLI: Line 96 in caf63cc As far as I know, it's not compatible with the Apache2 + MIT license. Maybe we need to open another issue to discuss this. What do you think? @mre |
06ac330 to
4455678
Compare
katrinafyi
left a comment
There was a problem hiding this comment.
makes sense and diff looks reasonable. would it be worthwhile to mention somewhere, maybe near the readme feature flags, that we use rustls and to report any issues or shortcomings?
also, idk much about TLS, but does this make it harder to use system-installed root certificates? this could affect people using lychee for intranet websites. but maybe the current vendored-openssl already makes that difficult - I'm not sure.
Sure, that makes sense.
I'm also no expert on that. But it seems like self-signed certificates are currently broken, so this PR shouldn't worsen the situation. I've asked on the issue if this branch resolves the problems, which is not entirely unlikely. |
| .client()?; | ||
|
|
||
| let response = client.check("https://example.com").await?; | ||
| let response = client.check("https://rust-lang.org").await?; |
There was a problem hiding this comment.
Was this change intentional? It seems to be unrelated to rustls and as far as I remember, the goal was to show how includes overrides excludes. By changing the URL this is no longer clear.
There was a problem hiding this comment.
Oh thanks for pointing out. Yes it was intentional but I forgot to update the includes and excludes. Fixed now.
I've replaced example.com with rust-lang.org because it might be confusing to people. (at least it confused me) Running cargo run --example builder makes the example panic. The URL is still excluded because we explicitly exclude example.com from checking.
We need to include the feature flag to make the example not panic: cargo run --example builder -F check_example_domains. With f4d1a38 the example will now pass independently of the provided feature flags.
Co-authored-by: katrinafyi <39479354+katrinafyi@users.noreply.github.com>
As recommended by @katrinafyi in lycheeverse#1928 (review)
Thanks, I agree :)
Done in e9d1e97 |
Closes #1721
Closes #1970
Closes #1920
Hopefully closes #2024
Background
Using Rustls instead of openSSL should simplify many things. Depending on platform specific external openSSL libraries seems to be more brittle than using Rustls for all targets. Very recently I've updated lychee to use reqwest 0.13.1 which coincidentally also uses Rustls by default now. This shows how Rustls should now be fully production ready and well tested. So now seems like a good moment to make the full switch.
As it is a breaking change anyhow I'd like to seize the opportunity to fully remove openSSL. If it works out well it should make maintenance easier. If it turns out that some people really need openSSL (which I think is unlikely) or run into issues we could still add a new feature flag before the next release.