actix / actix-net

A collection of lower-level libraries for composable network services.
https://actix.rs
Apache License 2.0
706 stars 345 forks source link

feat(actix-tls): support for rustls 0.23 #554

Closed SleeplessOne1917 closed 4 months ago

SleeplessOne1917 commented 4 months ago

PR Type

Feature

PR Checklist

Check your PR fulfills the following:

Overview

This adds a feature to allow users of this library to use rustls v0.23. I followed what was already done with v0.22 to figure out the changes I had to make. Once this makes it into a release, I plan on using these changes to make a similar change in actix-web to resolve problems like this issue.

SleeplessOne1917 commented 4 months ago

I'm not sure why only the Windows builds are failing.

asonix commented 4 months ago

tokio-rustls 0.26.0 defaults to enabling aws-lc-rs as the crypto backend instead of ring. On windows this doesn't build without nasm installed.

One solution could be to disable default features for rustls and tokio rustls so that the user can choose their preferred crypto provider

Another could be to disable default features and add a ring feature

robjtede commented 4 months ago

Lets just install nasm on Windows. See: https://github.com/robjtede/inspect-cert-chain/blob/e49cdf0bad5e65e6b3cc3b08096f35ad61aae3e2/.github/workflows/ci.yml#L34-L36

SleeplessOne1917 commented 4 months ago

Builds are still failing for msrv Windows (excluding MinGW). The CI log doesn't display anything useful, but I got this error running one locally:

  thread 'main' panicked at /home/insomnia/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aws-lc-sys-0.15.0/builder/main.rs:401:5:
  aws-lc-sys build failed. Please enable the 'bindgen' feature on aws-lc-rs or aws-lc-sys.For more information, see the aws-lc-rs User Guide: https://aws.github.io/aws-lc-rs/index.html

Since tokio-rustls doesn't allow us to include the bindgen feature, I'll make a PR on tokio-rustls to make it useable.

SleeplessOne1917 commented 4 months ago

@robjtede The 2 failing tests aren't marked as required, and they use the minimum supported rust version. Are there any other changes I need to make for this to be good to merge?

SleeplessOne1917 commented 4 months ago

@robjtede is this good to merge? I saw in the issue I linked in the PR description that rustls's different backends make using it difficult and that the maintainers haven't decided how to handle the problem yet.

Are the changes in this PR a sufficient stopgap for the time being?

asonix commented 4 months ago

IMO since actix requires a user-supplied ServerConfig it doesn't actually need to care about which crypto backend is in use, and can probably get away with disabling default features.

Tirka commented 4 months ago

If you stuck with following runtime panic after updating rustls to 0.23

thread 'main' panicked at /home/username/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.7/src/crypto/mod.rs:260:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Here's how I solved the problem:

// pick and uncomment preferred crypto provider
// use rustls::crypto::aws_lc_rs as crypto_provider;
// use rustls::crypto::ring as crypto_provider;
crypto_provider::default_provider()
     .install_default()
     .unwrap();

Put this code before creating rustls::ServerConfig