Rustls 0.20 and HttpsConnectorBuilder

[g2p]

This upgrades rustls to 0.20 (pending a release of rustls-native-certs), and ports the HttpsConnectorBuilder in #145 to something more like rustls' new ConfigBuilder (typestate based).

Parts of the rustls 0.20 work were taken from #153.

[g2p]

(Tweaked Cargo.toml in response to comments)

[LoganDark]

loving this, can't wait to use hyper-rustls once it supports the latest stuff

[paolobarbolini]

I had a chance to try the new rustls inspired API and I really liked it, although when rust-analyzer decides not to work it takes some time to combine things in the right order by going from the docs.

One more thing which would be cool to have is both the compiletime and runtime option to disable TLS 1.2. The compiletime option requires rustls and tokio-rustls to be imported without default features, adding the tls12 feature to hyper-rustls and enabling it by default. The runtime option is also necessary because as is explained in the rustls docs

due to the additive nature of Cargo features and because it is enabled by default, other crates in your dependency graph could re-enable it for your application. If you want to disable TLS 1.2 for security reasons, consider explicitly enabling TLS 1.3 only in the config builder API.

https://docs.rs/rustls/0.20.0/rustls/#crate-features

If you'd like to I can also implement it myself and send it in a separate PR after this one is merged

[LoganDark]

rustls-native-certs 0.6.0 has been published now: https://crates.io/crates/rustls-native-certs

[djc]

There's a lot of good stuff in this PR, but I think it has too much going on at once. I think this could be split in at least three parts, either separate commits in this PR or separate PRs:

1. Upgrade to rustls 0.20: this would have sort of the minimal work to enable rustls 0.20 support. I think the ConfigBuilderExt can probably be in scope for this, but miscellaneous refactoring in Service::call() should probably be minimized.
2. The ConnectorBuilder API.
3. Other refactoring should probably be split out in separate parts (including Service::call() changes to the extent not required for 1 or 2).

I also like @paolobarbolini's suggestion to configure TLS 1.2 support.

@g2p let me know if you have time to work on this soon, otherwise I can revive my own PR. (Also for now, I'd prefer to not split out config or builder into separate files.)

[g2p]

I've reworked the commit history to better separate @djc's point 1 (minimal changes for rustls 0.20) from 2 and 3.

The connector refactoring (point 3) is needed by point 2 to be consistent with how schemes are configured: I expose https_only and https_or_http, I've removed the existing behaviour which defaulted any non-https scheme to http (not documented and seemingly accidental). Finer grained scheme control will be possible to add in a backward compatible way, but it needs to be motivated by actual use cases (do we want to add https-equivalent schemes?)
If the changes to HttpsConnector::call seem spurious in the GitHub view, looking at them with git diff ignoring spaces should make them easier to review:

git log -p -b src/connector.rs

I've forwarded crates features (@paolobarbolini), tls12 and logging forward to those features in tokio-rustls and rustls. There's still some implicitness in whether TLS 1.2 is enabled at a distance through features, rustls configuration methods encourage this, it might be addressed through examples or a configuration method in rustls.

In practice: replacing .with_safe_defaults() with

with_safe_default_cipher_suites(&[
        &rustls::cipher_suite::TLS13_AES_256_GCM_SHA384,
        &rustls::cipher_suite::TLS13_AES_128_GCM_SHA256,
        &rustls::cipher_suite::TLS13_CHACHA20_POLY1305_SHA256])
    .with_safe_default_kx_groups()
    .with_protocol_versions(&[&rustls::version::TLS13])

Exposing a .with_tls13_defaults() method in rustls would be best, improving the ergonomics of rustls in the original crate rather than this one.

[g2p]

(Just fixed the --no-default-features --features=rustls-native-certs,logging combination which broke after making logging optional)

[djc]

Good stuff, thanks for your work on this! Sorry to drag this on, I have a few more questions.

Exposing a .with_tls13_defaults() method in rustls would be best, improving the ergonomics of rustls in the original crate rather than this one.

What scenario are you worried about, exactly? Users who inadvertently enable TLS 1.2? Folks who explicitly want to disable 1.2 should not call with_safe_defaults() but should instead specify the versions explicitly during the config builder setup; this doesn't seem so unreasonable.

[g2p]

I believe everything here is resolved, re alpn_protocols I'd much rather keep the implementation straightforward as it is (handle only what the builder understands, ie http1/http2 depending on feature gates), there may be a case for letting the user reorder things or add extra later, which is still available with the From<(H,C)> constructor but I don't want to add it to the builder interface now, I'm not seeing the use cases.

Instead of clearing the field, I can require alpn_protocols to be empty going in so that we can choose to handle it later without breaking API.

[djc]

Thanks, this is looking good!

[g2p]

Thanks for the reviewing!

[Gelbpunkt]

Hi there, I updated some of my code to use this and the latest changes are confusing me.
From what I see, this is valid code:

let connector = HttpsConnectorBuilder::new()
        .with_webpki_roots()
        .https_only()
        .enable_http1()
        .enable_http2()
        .build();

Yet it fails at the ALPN assertion. What am I doing wrong?

Edit: Removing .enable_http2() to only enable http1 results in hyper::Error(Connect, Custom { kind: Other, error: Custom { kind: InvalidData, error: AlertReceived(ProtocolVersion) } })

[g2p]

@Gelbpunkt thanks for the heads up, I just fixed the assert in #160.

The connection error might be related to your peer not handling TLS 1.3, did you enable the tls12 feature (part of the default set)?

[Gelbpunkt]

@Gelbpunkt thanks for the heads up, I just fixed the assert in #160.

The connection error might be related to your peer not handling TLS 1.3, did you enable the tls12 feature (part of the default set)?

Thanks, both of the configurations now work after also enabling TLS 1.2. I guess I didn't expect the remote to not support TLS 1.3 and didn't pay attention to the new feature flag. Thank you very much for the fix!