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

Add optional Rustls support #1099

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Add optional Rustls support #1099

merged 4 commits into from
Jun 16, 2023

Conversation

stefankreutz
Copy link
Contributor

Hello. I would like to discuss adding optional Rustls support to Lychee. I've prepared a pull request to show that's it's mostly a question of passing feature flags down to existing dependencies. What do you think?


This commit adds a non-default feature flag to use Rustls instead of OpenSSL.

My personal motivation is to use Lychee on OpenBSD -current, where the openssl crate frequently fails to link against the unreleased system LibreSSL. Using the vendored-openssl feature helps with compilation, but segfaults at runtime.

The commit adds three feature flags to the library, binary, benchmark, and all examples:

  • The native-tls feature flag toggles the openssl crate.
  • The rustls-tls feature flag toggles the rustls crate.
  • The email-check feature flag toggles the check-if-email-exists crate, which is the only existing functionality currently incompatible with Rustls.

By default, native-tls and email-check are enabled. Thus, Lychee (bin and lib) can be used as before unless default features are disabled.

To use the Rustls feature, pass --no-default-features --features rustls to cargo check/build/test/..., e.g.,

$ cargo clippy --workspace --all-targets --no-default-features --features rustls-tls -- --deny warnings

Checking email addresses requires both, native-tls and email-check, to be enabled. Otherwise, email addresses are excluded.

The email-check feature flag is technically not necessary. I preferred it over not(rustls-tls) because it's clearer and it addresses the AGPL license issue #594. As far as I understand, a Lychee binary compiled without the email-check feature could be distributed with file-based copyleft for the MPL-licensed dependencies only. But that's out of scope here.

The benchmark shows a performance regression varying between 2% and 4.4% when using Rustls instead of OpenSSL on my machine.

PS: The ring crate needs to be patched on OpenBSD 7.3 and later until the new xonly patches have been upstreamed, see the rust-ring port.

This commit adds a non-default feature flag to use Rustls instead of OpenSSL.

My personal motivation is to use Lychee on OpenBSD -current, where the
`openssl` crate frequently fails to link against the unreleased system
LibreSSL. Using the `vendored-openssl` feature helps with compilation, but
segfaults at runtime.

The commit adds three feature flags to the library, binary, benchmark, and all
examples:

- The `native-tls` feature flag toggles the `openssl` crate.
- The `rustls-tls` feature flag toggles the `rustls` crate.
- The `email-check` feature flag toggles the `check-if-email-exists` crate,
  which is the only existing functionality currently incompatible with Rustls.

By default, `native-tls` and `email-check` are enabled. Thus, Lychee (bin and
lib) can be used as before unless default features are disabled.

To use the Rustls feature, pass `--no-default-features --features rustls` to
cargo check/build/test/..., e.g.,

    $ cargo clippy --workspace --all-targets --no-default-features \ --features
    rustls-tls -- --deny warnings

Checking email addresses requires both, `native-tls` and `email-check`, to be
enabled. Otherwise, email addresses are excluded.

The `email-check` feature flag is technically not necessary. I preferred it
over `not(rustls-tls)` because it's clearer and it addresses the AGPL license
issue lycheeverse#594. As far as I understand, a Lychee binary compiled without the
`email-check` feature could be distributed with file-based copyleft for the
MPL-licensed dependencies only. But that's out of scope here.

The benchmark shows a performance regression varying between 2% and 4.4% when
using Rustls instead of OpenSSL on my machine.

PS: The `ring` crate needs to be patched on OpenBSD 7.3 and later until the new
xonly patches have been upstreamed, see the `rust-ring` port.
By default, reqwest uses the webpki-roots crate with Rustls, effectively
bundling Mozilla's root certificates.

This commit uses the rustls-native-certs crate instead to use locally
installed root certificates, to minimize the difference between the
native-tls and rustls-tls features.
@mre
Copy link
Member

mre commented Jun 14, 2023

Couldn't take a look at the code yet, but I like the idea and I'm all for supporting more platforms. Thanks for the efforts so far.

Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. I wonder if we should mention something in the docs (maybe a line or two in the README.md somewhere)?

pad = "0.1.6"
regex = "1.8.4"
reqwest = { version = "0.11.18", features = ["gzip"] }
reqwest = { version = "0.11.18", default-features = false, features = ["gzip", "json"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Was the json feature missing before? Was it somehow implicitly activated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I assumed "json" was a default feature of reqwest, but it is not. In fact, I cannot see how it worked before. There is no other reqwest feature that enables the "json" feature, and it is necessary for the lychee::archive::wayback::get_wayback_link function.

pulldown-cmark = "0.9.3"
regex = "1.8.4"
# Use trust-dns to avoid lookup failures on high concurrency
# https://github.com/seanmonstar/reqwest/issues/296
reqwest = { version = "0.11.18", features = ["gzip", "trust-dns"] }
reqwest = { version = "0.11.18", default-features = false, features = ["gzip", "trust-dns"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could potentially save a bit of compilation time.

@stefankreutz
Copy link
Contributor Author

Changes look good. I wonder if we should mention something in the docs (maybe a line or two in the README.md somewhere)?

Yes, I think we should document the feature flags somewhere. AFAIK there is no standard way to do this with rustdoc yet, see here.

I've pushed a commit that adds a section to the README and comments to the Cargo.toml.

Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks 🙏

@mre mre merged commit 7dd84f6 into lycheeverse:master Jun 16, 2023
@stefankreutz stefankreutz deleted the rustls branch June 16, 2023 11:22
@mre mre added the enhancement New feature or request label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants