Skip proxies with MITM root certificates by setting RUSTUP_USE_UNSAFE_SSL env var

[orthoxerox]

Of course, importing the actual root certificate would be better, but since this option will be used for corporate proxies like in #1542 and some proxies MITM only some URLs and curl can only replace root certificate bundles, I think it should be sufficient.

[kinnison]

Firstly, thank you for contribution to rustup, but I'm afraid I am fundamentally uncomfortable with this particular change. In corporate environments the computers should have their root certificate bundles updated otherwise there's plenty of other software which is going to get upset. To me, doing this "just" to rustup rather than fixing the broken system environment is simply papering over an issue.

However

If others feel that this is worthwhile then I will not block the functionality. Given that, before we can accept your change we'll need a few things doing:

1. The code commit needs to be reformatted with cargo fmt and amended, otherwise the CI won't pass.
2. We could do with a README update to document this since otherwise it won't help many people.
3. I'd like a treatment of whether or not there's anything we need to do to rustup-init.sh to also meet this capability
4. If at all possible, I'd like to see a proper test for this too

[orthoxerox]

@kinnison I agree that fixing the root certificates is fundamentally the better option, but on Windows they are extremely flaky. Browsers work fine, but maven and curl, for example, can't pick our corporate root certificate and must be fed a custom keystore. In addition, curl must be given all certificates at once (you can append certificates to reqwest), so with a proxy that sometimes uses the original and sometimes the corporate certificate managing the keystores is a real pain.

Regarding the todo list, I am not even sure how to test this. The test should basically self-sign a TLS certificate, spin up an HTTPS server and then test the connection, shouldn't it?

[kinnison]

Okay, I understand the pain, I guess I forget how Windows can muck things up. Regarding the test, sadly currently we use file:/// URIs for the dist server, which won't use the certificates. As such, the "if at all possible" is weak, I'd be okay with some demonstrated testing instead for now. But 1, 2, and 3 remain important :-)

[orthoxerox]

An update on the unit testing. Unfortunately, hyper doesn't support TLS on its server, so I went looking for another one. tiny_http does, but the support is broken, I had to build it from some fork that has a pending unmerged fix.

I'm pushing the fix mostly to check it against Travis CI, please don't merge it until I replace the server with a proper crate. If you have any advice, I'll eagerly welcome it.

[orthoxerox]

Well, that was quite a ride. After wrangling with error messages for types that were six lines long I've managed to replace my unit tests with something that needs only tokio. I think this PR is now ready for review.

[kinnison]

@orthoxerox thanks for that, I've been busy with work but I'll endeavour to give this a proper review within the next few days.

[orthoxerox]

@kinnison Thanks, I've updated the unit test to look less like "my first Tokio program".

[kinnison]

Over-all, this is a nice change I think. There's a few nits there to be picked, and a few points where I'd like some feedback. With those resolved I'd be inclined to recommend a merge.

[kinnison]

Are these long options reliable for all implementations of wget? E.g. does BSD have different arguments? I'll be satisfied if you say "yes" but I'd like to ensure you've checked.

[orthoxerox]

AFAIK wget is a GNU program, not a POSIX command that is different on other UNIX-like OSes. NetBSD, of example, works with wget 1.17, and this option has been there since 1.10

Basically, the answer is yes.

[ugexe]

BusyBox uses a non-standard wget. For instance: the -N flag does not exist in BusyBox wget. So one should not assume a single implementation of wget, nor that they all work the same.

[kinnison]

This seems like an unusual allow() to have here, under what circumstances is it needed?

[orthoxerox]

cargo test warns I don't use this function in my test files.

[kinnison]

If you don't use it, should we remove it rather than leave it there? Or is there perhaps one more test which could be added to make it not-dead code?

[orthoxerox]

This support file is used by both my tests and the existing tests that test partial downloads. I could split it, but it would duplicate the rest of the code.

[orthoxerox]

I've pushed the requested fixes. Should I keep them as a separate commit, or should I squash them?

[kinnison]

As a whole the work looks good. I've reviewed the PR updates you made and I like them.

Yes, squashing it together would be preferable, so that it's a single neat commit.

[orthoxerox]

All squashed and ready.

[bors]

☔ The latest upstream changes (presumably #1643) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

I would personally consider this a pretty delicate change for the obvious security implications. Cargo for example currently does not provide this functionality. I am not certain we want to merge this and it may wish for more discussion than just a PR

[nrc]

Yeah, I'm pretty uncomfortable about this. I would want somebody knowledgeable about the security implications to weigh in on the pros and cons

[orthoxerox]

@alexcrichton Both cargo and maven allow the user to specify a custom certificate bundle and are still a major pain to use on my employer's network. It's not their fault, of course, but I think cargo would be better served by supporting custom repositories and obtaining Artifactory/Nexus support, as that's the best way to get maven to work, for example.

Coming back to rustup, your corporate PC is already fully controlled by your employer. If you cannot trust them not to tinker with your downloads, you shouldn't work for them at all, as they can tinker with them any time they want.

[alexcrichton]

@orthoxerox yeah I think it's a missing feature that you can't switch the certificates in rustup, but rustup should remain consistent with Cargo in whether it allows ignoring SSL cert verification entirely.

[dwijnand]

cc @rust-lang/cargo and @rust-lang/dev-tools: if anyone has any expertise and/or a strong opinion on this change (and its relation to cargo) please let us know if you're for or against this change.

[joshtriplett]

I'm really uncomfortable with the idea of passing --insecure or --no-check-certificate or similar, rather than passing the expected certificate.

[ehuss]

I'm a bit confused on the fundamental issue. If the proxy is requiring a custom cert, does rustup not provide a way to use the system's local cert db? I was under the impression that curl uses the platform's native cert store for every platform.

[wbl]

Hi, I'm new to rust but have a history as a security reviewer including codiscovering RCE in OpenSSL. I do not think this patch is a good idea. Instead of trusting a corporate proxy it throws the barn door wide open: any possible MITM can start futzing with stuff. I would prefer to figure out why curl is not using the system trust store (a journey that is likely to be quite wonderful and hairy) and ensure it does.

[Manishearth]

(for context, given that this is blocked on security issues, I invited Rusty folks from a security community I participate in to add their two cents)

[kinnison]

@wbl Thank you for your joining the conversation, and thanks @Manishearth for inviting you.

I agree that it's definitely better to find a way to not need this. We recently switched from curl by default to reqwest by default. I wonder if there's a good way to improve matters via that perhaps?

[rbtcollins]

Hi, coming here after the weekly meeting.

I have two points to raise here.

Firstly, made already, disabling SSL verification is a significant risk even in a corp environment, lets not do that to our users. Permitting adding an additional root is much more modest.

Secondly, re @wbl's 'why isn't the default trust store used' - long historical reasons, heck, even today the rust openssl crate, which depends on Vcpkg installed OpenSSL (vcpkg being MS maintained) still uses its own trust store by default, and AIUI every application still needs to do its own glue code like so - https://stackoverflow.com/questions/9507184/can-openssl-on-windows-use-the-system-certificate-store - to integrate in.

Its possible that rustup 'just' needs such glue to get us to using the system store here, in which case great.

But, I would argue that permitting customisation of certificate stores is common across many many HTTP using tools - curl, wget, kubectl, the openstack clients etc etc etc. It permits running isolated environments side by side with no crosstalk concerns simply by providing different security contexts, and insisting that the system security store be the only store isn't a very strong argument IMO.

[MostafaNorzade]

I have strat Tor ( service tor start ) of course after installing the Tor.
Then install Rust.

[orthoxerox]

I'll close this PR since this is not the preferred solution. If I or anyone else comes up with a better fix, it should be a separate PR.