Support TLS v1.3 on all platforms

[mgoldenberg]

Note: this pull request has a companion pull request in the complement-crypto repository, which must be merged in conjunction with this one.

Before merging, this should be tested in conjunction with the Element X iOS client to ensure that TLS v1.3 is working properly. @stefanceriu has agreed to work on this.

Overview

The primary change in this pull request upgrades the reqwest dependency to its latest version, which defaults to using rustls with support for rustls-platform-verifier instead of native-tls (see reqwest@0.13.0). The benefit here is that rustls supports TLS v1.3 on all platforms, whereas native-tls does not.

Additionally, this pull request makes rustls the default TLS implementation in all the crates in this repository.

This will be particularly helpful with element-hq/element-x-ios#786.

Changes

• reqwest bumped to 0.13.1
  • The API for adding/replacing certificates has changed a bit, so this required some updating in HttpSettings::make_client
• oauth2-reqwest added in favor of oauth2/reqwest
  • This is required in order to be compatible with reqwest^0.13
  • oauth2-reqwest is currently in alpha release, so it probably makes sense to let this stabilize a bit. For details, see incompatibility to reqwest 0.13 ramosbugs/oauth2-rs#333 (comment).
• getrandom bumped to 0.3.4
  • This is required in order to be compatible with oauth2@5.1.0
• proptest bumped to 1.9.0
  • This is required in order to be compatible with getrandom@0.3.4
• Make rustls the default TLS implementation

Questions

Mirror feature flag names?

A number of feature flags have been replaced in the dependencies above.

1. reqwest/rustls-tls => reqwest/rustls - this is simply a name change, but is semantically identical (see reqwest@0.13.0).
2. getrandom/js => getrandom/wasm_js - the semantics here have changed slightly, but it seems to just make it easier to enable the wasm_js backend (see getrandom@0.3.4).

At any rate, I have updated references to these flags in each of the various Cargo.toml files, but have not changed the names of our exposed features to mimic those in the dependencies.

Any thoughts or preferences on whether to mirror those names? That would, of course, result in a breaking change.

Default to using rustls? Deprecate native-tls?

Now that the dependencies have all been bumped, we can use rustls on all platforms. Should this be the new default given that native-tls will very likely never support TLS v1.3 on Apple devices? And should native-tls be deprecated as a result?

UPDATE: The consensus here seems to be that we should default to using rustls, but that native-tls should still be available.

---

Fixes #5800.

• Public API changes documented in changelogs (optional)

Signed-off-by: Michael Goldenberg m@mgoldenberg.net

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.94%. Comparing base (29b506f) to head (8e33ea2).
⚠️ Report is 103 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/http_client/native.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6053      +/-   ##
==========================================
- Coverage   89.96%   89.94%   -0.02%     
==========================================
  Files         370      370              
  Lines      101930   101938       +8     
  Branches   101930   101938       +8     
==========================================
- Hits        91702    91693       -9     
- Misses       6678     6688      +10     
- Partials     3550     3557       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

Merging this PR will not alter performance

✅ 50 untouched benchmarks

---

_{Comparing mgoldenberg:support-tls-1.3 (8e33ea2) with main (40ff661)}

[ShadowRZ]

@mgoldenberg FYI for the current situation on oauth2-rs: ramosbugs/oauth2-rs#334 (comment)

[mgoldenberg]

@mgoldenberg FYI for the current situation on oauth2-rs: ramosbugs/oauth2-rs#334 (comment)

Thank you, I should have put an update here! The Rust team discussed this comment yesterday and decided we'd wait a little to see if any information is released about the new crate.

I'm planning to release a separate crate that provides a reqwest 0.13 client for oauth2 ¹

If movement is slow, we'll follow the author's prescription of implementing the traits ourselves.

as mentioned in the docs, any user of this crate can bring their own HTTP client by providing either a closure or an implementation of the AsyncHttpClient/SyncHttpClient traits. this solution can also be used to support a different version of reqwest/ureq/etc. if you need to upgrade to reqwest 0.13 urgently, I suggest taking that route. ¹

Footnotes

1. From comment ↩ ↩²

[Neustradamus]

Good news rust-native-tls has now the TLS 1.3 support!

Linked to:

• Add TLS1.3 support rust-native-tls/rust-native-tls#353
• Add a feature flag for forcing selection of openssl rust-native-tls/rust-native-tls#291
• TLS 1.3 version support rust-native-tls/rust-native-tls#278
• Enable TLS 1.3 rust-native-tls/rust-native-tls#235
• TLS 1.3 rust-native-tls/rust-native-tls#159
• Support TLS 1.3? rust-native-tls/rust-native-tls#140
• TLS 1.3 support retep998/winapi-rs#963
• Add symbols for TLS 1.3 retep998/winapi-rs#859
• TLS 1.3 Only olback/barium#14
• ElementX doesn't support TLS 1.3 element-hq/element-x-ios#786
• Support TLS v1.3 on all platforms #6053
• TLS 1.3 support? deltachat/deltachat-ios#1693

[frebib]

@mgoldenberg FYI for the current situation on oauth2-rs: ramosbugs/oauth2-rs#334 (comment)

Thank you, I should have put an update here! The Rust team discussed this comment yesterday and decided we'd wait a little to see if any information is released about the new crate.

I'm planning to release a separate crate that provides a reqwest 0.13 client for oauth2 1

There's an alpha implementation now ramosbugs/oauth2-rs#333 (comment)

[ShadowRZ]

@mgoldenberg As I explained in review, I don't think it's right to specify that aws-lc-sys doesn't use the OpenSSL license.

In fact it does have parts covered by the OpenSSL license via aws-lc project.

Could you please clarify why you did https://github.com/matrix-org/matrix-rust-sdk/pull/6053/changes/1b88edc8c6cc1e8470d09a165979af533a27d909..ec4f6af3acbcf827bde8b83e6e4e7d9ef0d9050e in the review?

[mgoldenberg]

@ShadowRZ, I'm not sure I see a review from you?

But at any rate, the Rust team recommended I use cargo deny's license clarification feature, instead of allowing OpenSSL licenses.

My understanding from the conversation was that the license could be recast based on the following statement from OpenSSL.

For the 3.0 release, and later releases derived from that, the Apache License v2 applies. This also applies to the git “master” branch.

Presumably this is preferable because adding a new license might involve some legal hurdles? I'm not terribly familiar with licensing issues, etc. so I'm mostly just following guidance here. I'd be happy to change the clarify clause into an allow clause, but would want @poljar to weigh in here.

[Hywan]

Thank you for the contribution! I would like to clarify why we have 2 versions of reqwest now? The number of dependencies have increased significantly, with a lot of duplicated dependencies. I reckon it's caused by the duplication of reqwest but I didn't check.

[niclashoyer]

@mgoldenberg

My understanding from the conversation was that the license could be recast based on the following statement from OpenSSL.

For the 3.0 release, and later releases derived from that, the Apache License v2 applies. This also applies to the git “master” branch.

that is correct regarding OpenSSL, but reqwest 0.13 is using rustls with aws-lc by default and aws-lc is based on a specific version of BoringSSL, which was based on OpenSSL < 3.0.

I raised the issue regarding reqwest / aws-lc again here: aws/aws-lc#2203 (comment)

My understanding is, that as aws-lc is largely a fork/extension of BoringSSL which was licensed with OpenSSL this still applies to aws-lc until the maintainers of aws-lc change their licensing or clarify on this matter (which aws/aws-lc#2203 is all about).

AFAIK the better option would be to not use reqwest with the default features, but try to use OpenSSL instead of rustls. I don't know if using native-tls is a viable option. After all it should be possible to check this using cargo deny.

Or just leave it as it is or set rustls-no-provider and any user of matrix-rust-sdk has to decide for themselves what kind of rustls backend should be used. E.g. using rustls-ring would be an Apache 2.0 based provider for rustls.

[poljar]

that is correct regarding OpenSSL, but reqwest 0.13 is using rustls with aws-lc by default and aws-lc is based on a specific version of BoringSSL, which was based on OpenSSL < 3.0.

That's information that is easy to miss, thanks for raising this. In that case we can't allow this license without becoming GPL-incompatible.

We'll have to pick a different default TLS backend.

[mgoldenberg]

The Kotlin build failure in CI is due to aws/aws-lc-rs#1006. Fixes have been released, and also some workarounds are mentioned. But, it seems the licensing issue might mean we won't depend on aws-lc-rs, so I will wait for that conversation to progress before pursuing a fix here.

[Hywan]

There we go. Can you rebase your fixup commits please?

[mgoldenberg]

There we go. Can you rebase your fixup commits please?

Rebasing the entire branch is a bit of a pain because of all the merges that have happened to keep this branch up to date, so I just reworded the fixup commit and rebased from there. If the commit history is feeling too messy, I can create a new branch with a cleaner history - just let me know!

[Hywan]

Looks good to me!