-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
[TLS] Disable TLSv1.3 support by default #102774
Conversation
This looks good to me, I think it's reasonable to default to TLSv1.2 if we have known issues with v1.3. I'd also be fine with #102770 if we prefer to anticipate it being merged upstream and try to have it working in Godot 4.4. The main incentive for going with #102770 is that if we release 4.4 with #102774 (defaulting to TLSv1.2), then we might have to stick to that default for the lifetime of 4.4, as changing defaults would be a compat breakage. While if we patch it up now, we can keep the default to TLSv1.3, and worse case have bugs in 4.4.stable, which we'd patch up in 4.4.1.stable without technically breaking compat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally on Linux, it works as expected. See also #102770 (review).
Considering TLS 1.3 isn't a hard requirement for most services (most still support TLS 1.2 due to legacy clients) and that TLS 1.2 doesn't have known exploitable flaws yet, it's probably fine. However, I'm also concerned about the default setting changes we'd potentially have to make in a patch release.
It may be worth pointing out the project setting in the HTTPClient and HTTPRequest class documentation still, in case someone is testing against a TLS 1.3-only service.
f10024e
to
10c2dd2
Compare
As discussed on RC, and given concerns expressed on #102770 being a bit too cutting edge, let's go with this approach for 4.4. |
Tested this, I get this warning spammed when I open the Asset Library in the project manager:
|
Without this PR, while browsing the Asset Library, I get errors like this:
I confirm that this PR (+ my patch above) fixes it. |
10c2dd2
to
488cdba
Compare
Thanks! |
mbedTLS does not support TLS Handshake record layer fragmentation which seems enabled on some sites (notably, Meta's sites).
This PR makes TLSv1.3 optional (and disabled by default).
We should decide if we want to proceed with #102770 (which incorporates an upstream, yet unmerged, PR adding support for the aforementioned feature in mbedTLS -> Mbed-TLS/mbedtls#9949) or merge this instead.
I also had another branch making the TLS min and max versions configurable via TLSOptions but I honestly think that's an overkill and adds a lot of API complexity for something that should just work out of the box.
Fixes #101910 .