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

Windows 7 users broken by recent Github change #5066

Closed
retep998 opened this issue Feb 23, 2018 · 37 comments · Fixed by #5097
Closed

Windows 7 users broken by recent Github change #5066

retep998 opened this issue Feb 23, 2018 · 37 comments · Fixed by #5097

Comments

@retep998
Copy link
Member

retep998 commented Feb 23, 2018

Issue was originally discovered in an unrelated issue: #5065

Due to a recent change on Github, TLS 1.0 and 1.1 are no longer supported: https://github.com/blog/2507-weak-cryptographic-standards-removed

Unfortunately Windows 7 WinHTTP by default only uses up to TLS 1.0, and because Cargo uses libgit2 which uses WinHTTP, this renders cargo effectively broken whenever it tries to fetch any git repo including the crates.io index.

PS [00:51:51] C:\Users\Arnavion\Desktop> cargo install https://github.com/alexcrichton/wasm-gc
    Updating registry `https://github.com/rust-lang/crates.io-index`
warning: spurious network error (2 tries remaining): unknown certificate check failure; class=Net (12); code=Certificate (-17)
warning: spurious network error (1 tries remaining): unknown certificate check failure; class=Net (12); code=Certificate (-17)
error: failed to fetch `https://github.com/rust-lang/crates.io-index`

Caused by:
  unknown certificate check failure; class=Net (12); code=Certificate (-17)

Fortunately this issue can be fixed by the user by ensuring they have a certain update and also editing their registry appropriately, as described here: https://support.microsoft.com/en-us/help/3140245/update-to-enable-tls-1-1-and-tls-1-2-as-a-default-secure-protocols-in

However users are not going to magically know what the solution is when they run into this problem, so we need to tell them everywhere possible. We need to spread awareness on this issue through the Rust blog, TWiR, Reddit, Twitter, and anywhere else that Rust has a social media presence.

Additionally we need to implement checks in Cargo and Rustup for users that are using Windows 7 and don't have the fix applied, telling the user very LOUDLY that they need to apply that fix, along with providing detailed instructions on how to fix it.

@Michael-F-Bryan
Copy link

However users are not going to magically know what the solution is when they run into this problem, so we need to tell them everywhere possible. We need to spread awareness on this issue through the Rust blog, TWiR, Reddit, Twitter, and anywhere else that Rust has a social media presence.

How easy is it to check the error type and try to see if a failure is due to this issue? If it's easy enough to detect we could even emit a helpful error message with a link to the relevant resources.

@aidanhs
Copy link
Member

aidanhs commented Feb 23, 2018

@aturon
Copy link
Member

aturon commented Feb 23, 2018

@matklad @Eh2406 is this something either of you would be up for addressing?

@matklad
Copy link
Member

matklad commented Feb 23, 2018

@aturon not sure I can really fix it, because I don't have the relevant windows experience. However, here are some pointers to different pieces of code which might be useful:

The network errors in Cargo are handled here:

git2::ErrorClass::Os => return true,
. I believe the suggestion by @Michael-F-Bryan would work: we need to compare ErrorClass and ErrorCode with those from the error report (12 and 17), and do something special.

However I am not sure that we can use just this code to correctly diagnose the situation. That is, we might give false positive for a genuinely invalid certificate? The relevant code from libgit is

https://github.com/libgit2/libgit2/blob/13c1bf0718363960c1867f35c9ce3ebc7bf74729/include/git2/errors.h#L45 (defenition of the error)

and

https://github.com/libgit2/libgit2/blob/408b16c1b06526f509fdb8997818d648d4d1ea1e/src/transports/winhttp.c#L263 (origin of the error for WinHttp).

I'll try to prepare a patch which checks for this specific combination of error code and OS and emits warning, but, as I've said, I am not sure that this won't give us false-positives :)

@setharnold
Copy link

Just curious, how will a cargo update that warns about this issue be distributed to the users that would benefit from it? Is it strictly for fresh installs?

Thanks

@matklad
Copy link
Member

matklad commented Feb 23, 2018

@setharnold most likely, it won't be :-)

I think we can and should backport this to current beta.

In theory, we might issue a Rust 1.24.1 as well.

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 23, 2018

Interestingly there may be a 1.24.1 for other reasons rust-lang/rust#48445

@retep998
Copy link
Member Author

An interesting point is that in order to detect that we are on Windows 7 we have to ask Windows what version it is, but Windows will lie and only report at most the newest version we claim to support in our manifest. Since cargo doesn't currently ship with a manifest (the Rust team really needs to stop ignoring this Windows specific stuff), this might make things a bit difficult.

@jethrogb
Copy link
Contributor

The way I read the linked Microsoft issue it's not really that WinHTTP on Windows 7 doesn't support TLS 1.2, it's just a matter of a wrong default configuration. Could we just patch libgit2 to do the right thing on Windows 7?

@retep998
Copy link
Member Author

retep998 commented Feb 24, 2018

cc @alexcrichton Since you maintain the Rust libgit2 stuff, do you know if its possible to get libgit2 to internally configure WinHTTP correctly so we don't depend on the user having to do registry edits?

@jethrogb TLS 1.2 support for Windows 7 still requires a rather recent patch and so even if we do fix that, we should still keep this error message because some people use Windows 7 with automatic updates disabled.

@matklad
Copy link
Member

matklad commented Feb 24, 2018

@retep998 filed an issue on libgit2 repo: libgit2/libgit2#4546

@glmdgrielson
Copy link

glmdgrielson commented Feb 25, 2018

Er, I have that update (3140245) and I'm still getting that stupid error. Deprecation was a stupid idea. What now?

P.S. it was a year ago. I don't think that counts as recent.

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 25, 2018

@glmdgrielson Do you have the DefaultSecureProtocols reg key set as discribed in https://support.microsoft.com/en-us/help/3140245/update-to-enable-tls-1-1-and-tls-1-2-as-a-default-secure-protocols-in?

matklad added a commit to matklad/cargo that referenced this issue Feb 25, 2018
@glmdgrielson
Copy link

I only have it through requesting admin rights. If there's a way to make it use TLS 1.2 without relying on default, I think that would be a better option.

@matklad
Copy link
Member

matklad commented Feb 26, 2018

Looks like this might get fixed on the libgit side: libgit2/libgit2#4550.

However, we still should give a warning for a case when automatic updates are disabled.

@ethomson
Copy link

Mea culpa - I spent most of last week dealing with this issue and thinking about how it affects my day job - Visual Studio / Git for Windows / Git Credential Manager for Windows, etc. I didn't have any spare cycles to think about how it would affect libgit2, but I should have been able to see this coming. My bad. 🙁

I do think that the Best Solution is still to encourage people to get to the latest updates for their OS and enable TLS 1.2 by default (via the registry key fix), however we are working on a fix for libgit2.

@driden
Copy link

driden commented Feb 26, 2018

I've done the update, checked the registry manually and still fails when trying to fetch the registry with the same error.
It's a bummer, was trying to set up my rust env here @work since I don't have that much on my plate atm.

@nairware
Copy link

@driden Same here. The proposed solution does not solve the problem for me (on my work laptop with Win 7 Enterprise 64-bit SP1).

I actually reinstalled rust from scratch today on my work laptop (usually I use my Windows 10 desktop at home for rust projects). My work laptop had rust 1.5 from December 2015, so I wanted to update to the latest version of rust today. To update, I had to reinstall rust from scratch (due to conflict with old rust and the new rustup). With the latest install, I should have the latest version of Cargo (mine says 0.24.0).

After I ran into the certificate issue with cargo, I first implemented the "easy fix" in the download first and rebooted--no fix. I then went and manually changed the two registry keys created from the fix (DefaultSecureProtocols; REG_DWORD; 0x00000A00) to only support TLS 1.2 (DefaultSecureProtocols; REG_DWORD; 0x00000800) instead of the combo of 1.1 and 1.2--and rebooted--no fix.

@matklad
Copy link
Member

matklad commented Feb 26, 2018

@nairware but do you have the actual update installed? My understanding is that easy fix is only a part of the problem: you must do both a) an update, b) the easy fix. I am not a windows user myself, so I can't really help to troubleshoot this further :-(

@ethomson
Copy link

@driden and @nairware, is it possible that you're actually going through some sort of enterprise proxy on your work computers? If there's something MITM'ing your TLS connections then it will need to be knowledgeable of TLS 1.2.

@driden
Copy link

driden commented Feb 26, 2018

@ethomson I am going through a proxy but there are W10 machines going through the same proxy with no issues whatsoever.
I've tried a couple of registry combos just in case one worked but still no luck.

@nairware
Copy link

@matklad The guide states two methods for solving the same problem. The guide explicitly states method 1 and method 2--not step 1 and step 2. Users should be able to perform either method 1 or method 2 to achieve the same end. I elected to perform the manual method (method 2) to attempt to solve the problem.

@ethomson I tried connecting to different wifi sources including my office wifi and my own personal wifi (cellphone as a hotspot) to bypass my company's firewall protected wifi. Regarding my laptop's security setup and software firewalls, anti-virus, etc., my setup is as simple and is unsecured as it could be. My machine is not even joined to the corporate Windows domain. I have my system explicitly and intentionally configured with minimal security features virtually all the time precisely for making moments like these easier to troubleshoot and endure.

@sfackler
Copy link
Member

@nairware It provides two ways to install the update, and then two ways to update the registry. You have to both install the update and update the registry.

@nairware
Copy link

@sfackler You are right. Somehow missed that download link twice, damn! Method 2 (implemented correctly :D ) works for me. I just did a successful update of registry 'https://github.com/rust-lang/crates.io-index'. Thanks all!

@nairware
Copy link

For others to emulate, my registry keys (created in each of the two directories) have the following values:

Name = DefaultSecureProtocols
Type = REG_DWORD
Data = 0x00000800 (2048)

bors added a commit that referenced this issue Feb 26, 2018
Warn Windows 7 users about old TLS

An eyepatch for #5066.

@retep998 what would the best way to check for windows 7 specifically? Currently I use just `#[cfg(windows)]`, which might give false positives on 8 and 10.
@driden
Copy link

driden commented Feb 26, 2018

@nairware OMG i missed it aswell. Just in case anyone wonders which one it is. https://www.catalog.update.microsoft.com/search.aspx?q=kb3140245 since I cannot use windows update (managed by sysadmins)

Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Feb 27, 2018
@matklad
Copy link
Member

matklad commented Feb 27, 2018

So the libgit2 is now fixed, here's the corresponding PR to the git2-rs: https://github.com/alexcrichton/git2-rs/pull/303

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Feb 27, 2018
bors added a commit that referenced this issue Feb 27, 2018
@IS4Code
Copy link

IS4Code commented Feb 27, 2018

Downloading KB3140245 and setting DefaultSecureProtocols seemed to help in my case.

@Scylardor
Copy link

Scylardor commented Feb 28, 2018

Fix confirmed on my Windows 7 by installing the update and manually entering the registry keys !

For those unfamiliar with Windows or the registry editor - here's how it should look if you did it right ! (But don't forget to do it at the two specified locations if you have a 64-bit OS).

top

(nevermind the French GUI)

bors added a commit that referenced this issue Feb 28, 2018
Revert "Warn Windows 7 users about old TLS"

We now have upgraded libgit2 version, so Cargo would use TLS 1.2
on windows unconditionally. Note that even *unpatched* windows 7
supports TLS 1.2.

@retep998, you've originally written that

>TLS 1.2 support for Windows 7 still requires a rather recent patch and so even if we do fix that, we should still keep this error message because some people use Windows 7 with automatic updates disabled.

However, it looks like Windows 7 **does** support TLS 1.2 out of the box, and the update is required to enable the use of TLS 1.2 by default:

libgit2/libgit2#4546 (comment)

My own testing on a random win 7 virtual box confirms this.

closes #5066
@tpdickso
Copy link

tpdickso commented Mar 11, 2018

I can confirm that despite being on the nightly channel, and having uninstalled and resintalled everything and getting the latest version of nightly through a fresh install of rustup last night, I was still receiving this error message without any indication as to how to solve it. Only today after finding this issue just now did I install the registry hotfix from the MS website and get cargo to function on my machine. I'm not sure why this issue is marked as closed.

image

This is what I see before, and after, installing the registry hotfix. I'm on Windows 7 with the following:
image

@retep998
Copy link
Member Author

@tpdickso Because the issue was fixed in cargo, but Rust itself has not yet updated its cargo submodule to get the version of cargo that has this issue fixed.

@jethrogb
Copy link
Contributor

jethrogb commented Mar 20, 2018

Another user running into this problem on u.r-l.o. They updated to today's nightly, which I'm pretty sure includes a recent enough cargo.

@felixholub
Copy link

What about users who do not have admin rights?

@kimtuck
Copy link

kimtuck commented Apr 14, 2018

I got this error today when trying to install something through cargo (command line was cargo install --git https://github.com/alexcrichton/wasm-gc).

I already had the patch from Microsoft installed (https://support.microsoft.com/en-us/help/3140245/update-to-enable-tls-1-1-and-tls-1-2-as-a-default-secure-protocols-in); so that is not a sufficient patch; I also needed to install the "Easy Fix" which is described towards the bottom of that page. Then the cargo command worked.

@squishy-clouds
Copy link

I am getting this issue on a Windows 8 (not Windows 8.1) computer. Its not my computer, so I can't try to fix it, but I didn't see anyone else mention Windows 8 and I thought I should say so on here.

@evertvr
Copy link

evertvr commented May 7, 2018

@kimtuck only installing the patch is indeed not enough. installing "Easy Fix" was also needed here.
Maybe something to add to the error message?

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 a pull request may close this issue.