search

Squirrel / Squirrel.Windows

An installation and update framework for Windows desktop apps
MIT License
7.23k stars 1.02k forks source link

TLS 1.3 support #1823

Closed sebastian-jtl closed 1 year ago

sebastian-jtl commented 1 year ago

Hard setting the security protocols in the ServicePointManager disallows the usage of newer security protocols for the update source. Enabling TLS 1.3 support in the app using Squirrel should now work as intended and not be overwritten whenever a web client is created.

caesay commented 1 year ago

TLS 1.0 and 1.1 are officially deprecated as being insecure. It's Microsoft's advice to disable them at the OS level and they are marked as deprecated within dotnet. Seems like it may be beneficial to leave them alone (eg. |= Tls 1.3 only & remove Ssl) instead of adding them back if the user (or dotnet) had already removed them.

sebastian-jtl commented 1 year ago

@caesay I would very much prefer to not play around with the ServicePointManager at all within Squirrel. This should be the responsibility of the actual app. The most sensible thing would be to just remove the call entirely, but that could be a breaking change and i opted for the "safest" route here to get TLS 1.3 working at all.

caesay commented 1 year ago

Well fundamentally I agree with you, Squirrel probably shouldn't be touching ServicePointManager. Where I disagree with you is the current implementation in this PR being "safe".

If the user/developer has already removed TLS1.0/1.1 in the startup of their app, using the following code for example:

ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;

As soon as Squirrel runs, it will add back TLS1.0 and 1.1 which are unsafe protocols. If Squirrel is going to touch the ServicePointManager at all, the only thing we should be doing is adding TLS1.2 / TLS1.3 and nothing else.

It's worth noting that the Clowd.Squirrel fork uses HttpClient instead of WebClient (which is also deprecated), and no longer updates the ServicePointManager at all.

sebastian-jtl commented 1 year ago

@caesay I meant "safe" as in the way that it does not change the current behavior of what security protocols will be available by default after the patch, not as in security. I would fully support completely removing the call.

And nice i did not know about that fork. I will look into it.

robmen commented 1 year ago

@sebastian-jtl This is the correct change to not make a breaking change to Squirrel. I appreciate your consideration.