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

Force highest TLS version supported #1716

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Force highest TLS version supported #1716

merged 1 commit into from
Mar 26, 2019

Conversation

sanmai-NL
Copy link

@sanmai-NL sanmai-NL commented Mar 17, 2019

The integrity and confidentiality of the installer script currently hinges on TLS. It is important to enforce the highest version of TLS in the instructions. Also, enforce the https scheme. Should redirects occur in the future, then each URL redirected to must be accessed using TLS 1.2 with HTTP, rather than allowing a plain HTTP link in the chain.

@kinnison
Copy link
Contributor

Hi @sanmai-NL,

Thank you for your contribution to Rustup. Would it also make sense to adjust rustup-init.sh similarly?

@sanmai-NL
Copy link
Author

Yes, and https://www.rust-lang.org/learn/get-started as well.

As for backward compatibility of a similar change to rustup-init.sh: I cannot determine when --tlsv1.2 was added to cURL. It could have been added in 2001 already, but I'm not sure based on the cURL changelog. I have successfully used these CLI options for about 5 years on production systems, though.

@kinnison
Copy link
Contributor

If you can verify that the cURL version in RHEL 7 and later distros is suitable then please update your patch to cover rustup-init.sh and we'll probably merge it. You'll need a separate issue opening on the website repo for that bit though.

@sanmai-NL
Copy link
Author

The integrity and confidentiality of the installer script hinges currently on TLS. It is important to enforce the highest version of TLS in the instructions. Also, enforce the `https` scheme. Should redirects occur in the future, then each URL redirected to must be accessed using TLS 1.2 with HTTP, rather than allowing a plain HTTP link in the chain.
@sanmai-NL
Copy link
Author

@kinnison: Done. Also added similar options to the wget invocation. According to the changelog, they support these parameters since 2014.

@tesuji
Copy link
Contributor

tesuji commented Mar 20, 2019

I afraid that we cannot use rustup in Ubuntu 14.04 (at least in Travis CI).
Edit: Looks like I am wrong. curl manpages of Ubuntu 14.04 lists those options.

@sanmai-NL
Copy link
Author

As for cURL, --tlsv1.2 was introduced in 7.34 and --proto in 7.20.2 according to https://curl.haxx.se/docs/manpage.html. Ubuntu 14.04 has an official cURL package based on 7.35. You can choose a more up-to-date Ubuntu edition (Xenial) on Travis CI.

@kinnison
Copy link
Contributor

We try to maintain an older CI baseline because it increases our reachability wrt. distributions - glibc versions are a pain.

@sanmai-NL
Copy link
Author

@kinnison: alright, but this is ready for merging no?

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if noone objects in terms of support for other OSes, I'll merge in the next few days.

@sanmai-NL
Copy link
Author

@kinnison: ping.

@kinnison
Copy link
Contributor

@sanmai-NL Our working group meeting is tonight, I planned to check there before hitting merge.

@kinnison kinnison merged commit bb05ccc into rust-lang:master Mar 26, 2019
@tesuji
Copy link
Contributor

tesuji commented Mar 26, 2019

@kinnison I've just noticed that command may not work on zsh. You have to type:

curl --proto \=https --tlsv1.2 -sSf https://sh.rustup.rs

or

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs

instead.

@kinnison
Copy link
Contributor

Hmm, I wonder why I didn't spot that..

rummages

Looks like I have setopt no_equals in my configs. Well spotted. Will you provide a PR to fix that please, and I'll merge it.

@@ -36,7 +36,7 @@
then follow the onscreen instructions.
</p>
<p>If you're a Windows Subsystem for Linux user run the following in your terminal, then follow the onscreen instructions to install Rust.</p>
<pre>curl https://sh.rustup.rs -sSf | sh</pre>
<pre>curl --proto =https --tlsv1.2 -sSf https://sh.rustup.rs | sh</pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be left as it was? This is just intended to be a quick one-liner copy/paste snippet and expanding with so many options can be pretty intimidating as a copy/paste snippet. Does this really add enough benefit relative to what it's displaying on the site?

Copy link
Author

@sanmai-NL sanmai-NL Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are expressing certain feelings towards this without backing them up with a UX study. I am sad you prioritize the length of an already technical one-liner over the clear and specified security improvement. I think it is instead a good signal that in the Rust ecosystem, security is taken seriously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly the display isn't as good as it used to be. We could use web techniques to hide the extra arguments from the display but include them in the copy/paste, and provide a copy-command-to-clipboard button which puts it all there, for ease of use?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would that be important?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the box requires scrolling, and people are more likely to fail to copy the whole command than not. While I personally dislike the narrow styling, I understand why it's there so would prefer to maintain it over trying to do better myself. If someone else could make it work somehow then that'd be good too.

rustup-init.sh Show resolved Hide resolved
mechie added a commit to mechie/misc that referenced this pull request Nov 19, 2020
Forces use of https and TLS1.2 to avoid shifty business.
See rust-lang/rustup#1716 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants