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

Use installation instruction with rustup as recommended in forge and rustup #1823

Closed

Conversation

yerke
Copy link

@yerke yerke commented Jun 17, 2023

This PR updates installation instruction with rustup to the one recommended in forge and rustup.

rustup-init.sh will try to enforce TLS 1.2 or higher if it is supported by curl. If it is not supported, it will print out the following warnings:

Warning: Not enforcing strong cipher suites for TLS, this is potentially less secure
Warning: Not enforcing TLS v1.2, this is potentially less secure

Adding --proto '=https' --tlsv1.2 arguments as we have right now does not do anything at all.

Both forge and rustup recommend using rustup with curl https://sh.rustup.rs -sSf | sh.

Fixes rust-lang/rust-forge#684

@yerke yerke requested a review from a team as a code owner June 17, 2023 06:54
@yerke
Copy link
Author

yerke commented Jun 17, 2023

@rbtcollins and @hi-rustin Can you please check this as members of Rustup team? Thanks.

@rust-lang/release Can you also please take a look, since @Manishearth pinged you in a somewhat related PR #1670? Thanks.

@Manishearth
Copy link
Member

https://rustup.rs/ itself recommends these flags; I don't consider forge to be authoritative. I believe these flags were added for redundancy because people were complaining about the possibility of attacks, and if the flags aren't there then people won't easily know about the mitigations in rustup-init.sh.

Happy to land this if the rustup team thinks we should, but currently I lean towards leaving it as is.

It is worth trawling the history of this website and rustup.rs to figure out when and why these flags were added.

@rbtcollins
Copy link

rustup perspective: I don't think we have a principled stand here and would be willing to change if a reasoned analysis thought we get a better position vis-a-vis end user security by changing (and recognising that security is a socio-technical problem).

However at this point I think the flags do make sense because they help ensure that the user is getting the software we published (not rustup, the rustup installer script).

This is the origin of the command line in Rustup

Author: Sander Maijers <[email protected]>
Date:   Sun Mar 17 12:45:43 2019 +0100

    Force highest TLS version supported
    
    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.

There are two MITM attacks possible on curlsh idioms: the first script download, and then the connections the installed script requests.

The warning you describe comes from the installer script detecting that it can't force validation of the downloaded rustup binary, which is pretty poor. We should perhaps make that a hard error at this time - 4 years later.

Removing the tls 1.2 forcing from the curlsh idiom means that misconfigurations could end up in an HTTP request in a redirect chain, which would permit trivial MITM malware injection.

I think we should probably look at updating to requiring TLS 1.3 at this point, but thats a different discussion.

@rbtcollins
Copy link

In fact thinking about it, whatever platform you're on, please file a bug on rustup about seeing that warning, so we can discuss how to get you integrity protection over both the installer and rustup itself.

@yerke
Copy link
Author

yerke commented Jun 17, 2023

@rbtcollins Got it. I understand your point and agree with it. I was looking at rustup-init.sh behavior instead of how the customer will be getting rustup-init.sh in the first place. I will open PRs to fix the recommendations in forge and rustup repos instead.

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.

Slightly different rustup instructions compared to rust-lang.org's Install page
3 participants