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

Consider switching to rustls as the default, and make openssl optional #2025

Open
nyurik opened this issue Nov 9, 2023 · 9 comments
Open
Labels
B-breaking-change Blocked: breaking change. B-rfc Blocked: Request for comments. More discussion would help move this along.

Comments

@nyurik
Copy link
Contributor

nyurik commented Nov 9, 2023

I would like to propose Reqwest switches to rustls crate, probably rustls-tls-native-roots variant of it, as the new default option for tls. Rustls seems like a highly stable and secure product, and most importantly - it does not require to compile non-rust code during the build process. This would obviously be a breaking change, and would have to be discussed and coordinated appropriately. Thx!

@gibbz00
Copy link
Contributor

gibbz00 commented Nov 12, 2023

Linking a good summary on the pros and cons of webkpi-roots vs native-certs from @djc himself (for anyone interested):

rust-lang/rustup#3400 (comment)

@djc
Copy link
Contributor

djc commented Mar 6, 2024

I (obviously) think it would be great to consider this for a next semver-breaking change.

Should consider this in tandem with the decision what rustls crypto provider to enable by default (see #2136).

@seanmonstar seanmonstar added B-rfc Blocked: Request for comments. More discussion would help move this along. B-breaking-change Blocked: breaking change. labels Mar 6, 2024
@seanmonstar
Copy link
Owner

seanmonstar commented Mar 20, 2024

I'll include some thoughts in here which after some discussion, could probably become better documented "design principles".

  • Regarding rustls as default, I think it's similar to whether to have hickory-dns by default. In that, I think both of those are usually better options, but there are obscure cases where they won't work, so currently reqwest defaults aim to work in more cases.
    • I do think that principle is worth solidifying and documenting (and arguing/discussing): should reqwest defaults aim for "works in strictly more cases", or "works better in the most common cases"?
  • Another point: native-tls should mean smaller binaries, since it uses what's already on the system (unless you use vendored).

@djc
Copy link
Contributor

djc commented Mar 20, 2024

I do think that principle is worth solidifying and documenting (and arguing/discussing): should reqwest defaults aim for "works in strictly more cases", or "works better in the most common cases"?

This feels like a simplistic way of trading off non-functional requirements; if the latter option is 100% better in 99% of cases, would you still pick the option that works in more cases? I'd argue there's some kind of expected value thing here which requires coming up with a weight for any number of non-functional requirements, like maybe:

  • Reliability: unlikely to break
  • Performance: works fast
  • Ergonomics: ease of integration

It looks like native-tls doesn't support TLS 1.3 which seems likely to affect performance and security and it uses OpenSSL on Linux which has a pretty spotty security history. rustls doesn't support TLS 1.1 (and earlier) which are officially deprecated anyway but which I guess could come up but should be increasingly rare. Also at this stage I think rustls is in a really good situation in terms of maintenance with two full-time maintainers so it's likely that issues (that are not by design) can fixed very quickly.

I would also argue that, as a Rust HTTP client built on hyper, a Rust HTTP protocol stack, all other things being equal it is nicer for integration and ergonomics (building etc) to use Rust for as much of the internals as possible, and doing so will improve quality of the Rust components over time because they get more exposure (many eyes, shallow bugs).

(I find the case for trust-dns a little harder to make because it is more resource-constrained on maintenance.)

As for binary size: I think it is already the cases that users who are sensitive to binary size are more likely to pick other clients (like ureq or curl), so I'm not sure it should be an important driver.

@seanmonstar
Copy link
Owner

This feels like a simplistic way of trading off non-functional requirements

I mean yea, including more criteria can help make a better decision. It was just an axis I started with.

(It looks like native-tls supporting TLS 1.3 could be close, FWIW.)

But I support much of what you said regarding rustls.

all other things being equal it is nicer for integration and ergonomics (building etc) to use Rust for as much of the internals as possible

In general I'd agree with the sentiment, but in this case, I think native-tls actually has a better story here for most targets. Most users won't have to compile anything "not Rust". With rustls, whether using ring or aws-lc, you need to compile C. rustls' new default backend requires having cmake. I don't think those things are the end of the world, but they do add some friction.


I'll also add, the way reqwest's features are structured, where default-tls only exposes things supported by both backends, and needing to enable either native-tls or rustls-tls to access specific things... that was with the intention of being able to eventually switch the default to rustls. It is something I'd like to do. I just wanted to make sure it's good for the most amount of people, not just what I like.

@djc
Copy link
Contributor

djc commented Mar 20, 2024

With rustls, whether using ring or aws-lc, you need to compile C. rustls' new default backend requires having cmake. I don't think those things are the end of the world, but they do add some friction.

That's fair! FWIW, I'm not a fan of the decision to make aws-lc-rs the default crypto provider for rustls -- I think the additional friction isn't worth the benefits for most downstream users. But of course reqwest could just decide to keep ring as the default for rustls' crypto provider.

It is something I'd like to do. I just wanted to make sure it's good for the most amount of people, not just what I like.

Sure, I think you're making valid points -- I just also want to argue the other side.

@seanmonstar
Copy link
Owner

As a general call for comments, if you (any of you) need native-tls over rustls, or if the default change were to harm you, or you know of specific situations as such, please chime in here.

@ofek
Copy link

ofek commented Oct 13, 2024

Is there a feature available that enables this? https://github.com/rustls/rustls-platform-verifier

@djc
Copy link
Contributor

djc commented Oct 13, 2024

@ofek see #2159.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-breaking-change Blocked: breaking change. B-rfc Blocked: Request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests

5 participants