goatcorp / FFXIVQuickLauncher

Custom launcher for FFXIV
https://goatcorp.github.io/
GNU General Public License v3.0
2.85k stars 333 forks source link

Could not create SSL/TLS secure channel #1026

Open HQuest opened 2 years ago

HQuest commented 2 years ago

Update disclaimer

What did you do?

The hardcoded definition for Windows 7 to use TLS v1.2 is causing something on Win11 to break the auto updates ("Could not create SSL/TLS secure channel"). A possible (but untested) fix is to isolate this TLS hardcode for Win 7 machines only - https://github.com/goatcorp/FFXIVQuickLauncher/blob/f9dcbc171fac8cfca5c5ed7ae69095bd8a299a4f/src/XIVLauncher.Common/Dalamud/DalamudUpdater.cs#L165 should read something like

    if (System.Environment.OSVersion.ToString() == "6.1")
    {
        ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;
    }

Platform

Windows

Wine/Proton runner version

No response

Relevant log output

No response

HQuest commented 2 years ago

Microsoft has ended mainstream support of Windows 7 in 2020 (https://docs.microsoft.com/en-us/lifecycle/products/windows-7), Windows 8.1 in 2018 and will end Extended support in 2023 (https://docs.microsoft.com/en-us/lifecycle/faq/windows). If the above code won't be fixed to properly limit its coverage only to what it is proposed, why are we supporting an unsupported OS and affecting new OS? As someone stated on Discord, "there might be something on my system", that might be true, however it is proven by the above code my system is fixed without the Windows 7 "fix".

goaaats commented 2 years ago

I've never heard of this before and you seemingly are the only person to have a problem with this on Windows 11 - a lot of our userbase already is on that OS. I don't like to change things in that part of the codebase if there's no clear advantage and I'm not convinced/don't see how this could possibly cause problems on Windows 11. You should try to reproduce it with another application. I recommend creating a minimal .NET 4.7 sample with the same SecurityProtocol code and a request to the same server. Let me know if you still have the issue.

HQuest commented 2 years ago

Not trying to pick up a fight but you are asking a diesel mechanic to do heart surgery. I do have the basics of programming, but I am in no way a programmer - I can't code any samples. Had you asked me about data from the OSI layers before, as an infrastructure person I would happily provide you with a ton of data. As someone from global scale operations background, I fully understand your stance of not willing to make changes because of one single person.

What I don't understand is the reluctancy of maintaining the right code. Even if I am the sole person in the world that faces this error, this section of the code is also questionable. Its comment states "GitHub requires TLS 1.2, we need to hardcode this for Windows 7" (emphasis mine), and yet the code applies such setting for any OS versions, regardless of being Windows 7 or not. Therefore, either the comment or the code is wrong. Yes, it sets one of the two currently supported cryptographic mechanisms in this application, however what happens when TLS 1.2 isn't the standard any longer and as previous, has to be superseded by TLS 1.3? Code has to change - or code has to be, as recommended by Microsoft, let for the OS to decide:

https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls

Consider the following recommendations:

For TLS 1.2, target .NET Framework 4.7 or later versions on your apps, and target .NET Framework 4.7.1 or later versions on your WCF apps. For TLS 1.3, target .NET Framework 4.8 or later. Do not specify the TLS version. Configure your code to let the OS decide on the TLS version. Perform a thorough code audit to verify you're not specifying a TLS or SSL version. When your app lets the OS choose the TLS version:

It automatically takes advantage of new protocols added in the future, such as TLS 1.3. The OS blocks protocols that are discovered not to be secure.

(some more emphasis mine)

Add the fact this code section was added perhaps when Windows 7 was still supported, it has been discontinued by the manufacturer for about 2 years now and this code should had been considered legacy by now, to avoid exactly this type of discussion. Quite frankly, I can continue to compile XL manually with this line changed - until a second, third or more comes around with the same glitch, and maybe this is re-evaluated by the group.

Feel free to take actions as you see fit, thank you and respect for developing this tool.

reiichi001 commented 2 years ago

Hello! Here's a couple of really basic sample applications for some SSL testing, one for .Net 4.7.1 (like XIVLauncher) and one for .Net 5 (like Dalamud). Source code if you'd prefer to build them yourself: https://github.com/reiichi001/SSL_Checker

SSL_Checker_Net5.zip SSL_Checker_Net471.zip

HQuest commented 2 years ago

Thank you @reiichi001. Ran your provided compiled code and both tests passed. See below. image