Re-add rustls support (enabled by setting RUSTUP_USE_RUSTLS)

[est31]

rustls is seen as mature enough for curl to depend on it optionally,
and it recently has had an audit.

This commit adds back rustls support removed by 86bb185

You can opt into using rustls by setting the RUSTUP_USE_RUSTLS env variable.

Blockers:

• Ring not compiling on apple silicon
• Ring release with the apple silicon fix
• Reqwest support for native certificate store
• Reqwest release with the native cert store support

TODO:

• Default should remain curl for now, but allow testing of rustls e.g. via RUSTUP_USE_RUSTLS

Fixes #568

[est31]

I haven't added ability to use the native TLS stack via an env var, because reqwest lacks support for that (it will just always prefer the native TLS stack over rustls), and because users wanting to use openssl already have an option with the RUSTUP_USE_CURL env var.

[est31]

If you want env var configurability, please leave a comment, because it'll require reqwest changes.

[est31]

Mhh seems that env var configurability was easier than thought.

[est31]

Anyone know what the cause of this can be? It only occurs on mac OS:

error[E0463]: can't find crate for `serde_derive`
   --> /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.115/src/lib.rs:285:1
    |
285 | extern crate serde_derive;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to previous error

[est31]

Apparently doing cargo update -p serde fixes the issue, no idea why

[est31]

Oh the mac OS bug seems to be addressed by @ehuss in #2513

[est31]

Alright as the other PR has a proper fix for the issue I've removed the local hack and leave the fix for @ehuss 's PR.

[kinnison]

Thank you for this work - have you confirmed that rustls will use the system certificate store properly? One issue which people using rustup behind corporate proxies complain about is that they have https problems. Making it harder for them to fix in a system-standard way would be bad.

[est31]

Currently the rustls mode of reqwest uses the mozilla provided certificate store through the webpki-roots crate. One would have to use the rustls-native-certs crate instead. This requires reqwest changes I think.

Note there are cons of using native certificates as well. rustup's entire security model depends on https only (cc #2028). But I guess that's a separate issue from rustls adoption. If rustup used curl previously, it can now use rustls.

[kinnison]

How does webpki-roots work wrt. updates? Do we have to release a new rustup build to get them?

[est31]

@kinnison yes, an update would be needed to get newer root certificates. I think there is no way around a mode for using the OS certificate store.

[est31]

rustls/rustls-native-certs#12

[kinnison]

Hmm, I think I need to spend more time thinking on this than I have right now. Thank you for that link though.

@rbtcollins what do you feel about this PR?

[est31]

Ok this should be the reqwest change: seanmonstar/reqwest#1058

Ignore the rustls-native-roots PR. It won't be required to switch rustup to rustls.

I've updated this PR to use my reqwest fork so that users can test this PR.

@kinnison do you know the users with a proxy setup that relies on OS native certificates? Could you maybe ask them to try this PR whether there is any regression for them?

Should rustls maybe not be enabled by default for now to enable a testing phase?

[rbtcollins]

I think this is a feature whose behaviour is hard to verify in direct testing; as such I'm very much in favour of having it working but not default in at least one release cycle, and encouraging testing of it before we switch over. Re: native certs - yes, we explicitly should use the native trust store: - rustup releases rarely, because: - rustup releases are expensive and tricky to do - making it a security driven issue to do more will put stress on systems that are not yet ready to be driven fast

…

-Rob

[kinnison]

This branch is currently in conflict with Cargo.toml. Could you rebase, check things are still clean, and then ping me for re-review?

[est31]

@kinnison I've rebased it but the FIXME's are still unfixed, so CI will fail, and it will still use a forked version of upstream reqwest :).

[kinnison]

@est31 Thanks, it's okay I just wanted to be sure the diffs were clean so I can think about things. Obviously I agree it's not mergeable yet :D

[est31]

All the blockers are resolved. Ready for review now @kinnison !

[kinnison]

I believe this is okay, and default to "not rustls" @rbtcollins How do you feel about this? It's easily backed out if it proves out as bad, but it shouldn't be harmful to include this as optional support. I'm not interested in documenting this publically yet though.

[rbtcollins]

Default off with no new dyn depends is fine with me

…

On Sat, 21 Nov 2020, 22:42 Daniel Silverstone, ***@***.***> wrote: ***@***.**** commented on this pull request. I believe this is okay, and default to "not rustls" @rbtcollins <https://github.com/rbtcollins> How do you feel about this? It's easily backed out if it proves out as bad, but it shouldn't be harmful to include this as optional support. I'm not interested in documenting this publically yet though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2517 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZ7XQTQYN6YQFYNOWUNQDSRAX3TANCNFSM4SKDUCSA> .

[est31]

@kinnison why do you not want to document it publicly? I think it should be so that users can be asked to try it out as the intention is to make it default some point in the future.

[kinnison]

@est31 Sorry, I was perhaps not clear. My intent would be that after 1.23 we merge this. Then we can seek targetted feedback by asking for people to build master and try it. When we're confident there's no reason to exclude it from a release we can release with it in, but disabled. At that point we seek further engagement with the feature, and document it in our book.

Given we're still not comfortable disabling CURL support I'm really going to be dealing with this cautiously.

[est31]

Oh so that's your plan. Note that it's entirely opt-in. I'm not sure what could go wrong if you don't enable it (the PR makes it opt-in at runtime), so I'm not sure something is gained by that proposal. But shrug. Better it gets in later than never :).

[est31]

Oh hmmmm .... the new release is in staging already. Yeah then it's better to wait for the one after that. https://internals.rust-lang.org/t/new-rustup-release-staged-and-needing-testers/13446

[est31]

Rebased onto the 1.23.0 release

[kinnison]

I think it's time to take this on and see how we do. @est31 it would be good if you would switch your local use of rustup to use this feature so we can check it more thoroughly. I shall endeavour to do so as well.

[est31]

@est31 it would be good if you would switch your local use of rustup to use this feature so we can check it more thoroughly.

Good idea! I assume you install rustup locally by doing cargo build --release && cp target/release/rustup-init ~/.cargo/bin/rustup?

[kinnison]

You can do cargo run -- --no-modify-path -y or cargo run --release -- --no-modify-path -y if you want to install the build.