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

[mbedTLS] Enable TLS 1.3 support #96394

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Aug 31, 2024

Move library initialization to module registration functions.

Only set library debug threshold when verbose output is enabled.

TLSv1.3 functions seems to be a bit more verbose then expected, and generate a lot of noise. Yet, some level of debugging without recompiling the engine would be nice. We should discuss this upstream.

Draft status:

Fixes #92101

Test:

# main.gd
extends Node

var http : HTTPRequest

func _ready() -> void:
	http = HTTPRequest.new()
	add_child(http)
	http.request_completed.connect(_completed)
	if http.request("https://tls13.akamai.io/") != OK:
		_exit()

func _completed(result: int, code: int, headers: PackedStringArray, body: PackedByteArray):
	if code != 200:
		print("Request failed, result: %d, code: %d" % [result, code])
		_exit()
		return
	var str_body := body.get_string_from_utf8()
	var tls13 := "Your client negotiated TLS 1.3" in str_body
	var tls12 := "Your client negotiated TLS 1.2" in str_body
	if tls13:
		print("TLS 1.3 supported :)")
	elif tls12:
		print("TLS 1.3 **NOT** supported :(")
		print("TLS 1.2 supported :)")
	else:
		print("Can't determine TLS version... Check yourself: %s" % str_body)
	_exit()

func _exit():
	get_tree().quit.call_deferred()

Note for package maintainers: When using builtin_mbedtls=no TLS 1.3 support will depend on the system library version and configuration.

@fire
Copy link
Member

fire commented Sep 3, 2024

The cicd check is failing on The command line is too long..

@Faless
Copy link
Collaborator Author

Faless commented Sep 3, 2024

The cicd check is failing on The command line is too long..

Yes, seems like a build system issue on Windows + MSVC (not MinGW apparently).

I don't know how to fix that and I currently can't easily test MSVC builds, so help is welcome.

@akien-mga
Copy link
Member

The cicd check is failing on The command line is too long..

Yes, seems like a build system issue on Windows + MSVC (not MinGW apparently).

I don't know how to fix that and I currently can't easily test MSVC builds, so help is welcome.

CC @bruvzg @Repiteo

@bruvzg
Copy link
Member

bruvzg commented Sep 25, 2024

I don't know how to fix that and I currently can't easily test MSVC builds, so help is welcome.

Probably in the same way as ar - #96407, MSVC lib do support running with response file - https://learn.microsoft.com/en-us/cpp/build/reference/running-lib?view=msvc-170#lib-command-files

@Faless
Copy link
Collaborator Author

Faless commented Sep 25, 2024

Probably in the same way as ar - #96407, MSVC lib do support running with response file

I remember trying it but it didn't work. I added 3c66820 and indeed it's still failing (doesn't seem to use the tempfile).

Am I doing something wrong?

@bruvzg
Copy link
Member

bruvzg commented Sep 25, 2024

Am I doing something wrong?

It's actually already is using TEMPFILE by default, the issue is env["MAXLINELENGTH"] = 8192 line in our config (added in #87154), removing it fixes the build.

@bruvzg
Copy link
Member

bruvzg commented Sep 25, 2024

For the reference, max length seems to be 8191 - https://learn.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation#more-information, but it's not working either. 8000 seems to working, so not sure what's the real limit is. Scons default is 2048.

@Faless
Copy link
Collaborator Author

Faless commented Sep 25, 2024

Opened #97458 with the windows fix.

Thanks for the help, this was driving me crazy

@Faless Faless removed the needs work label Sep 25, 2024
@DjSapsan
Copy link

OMG you are hero!
I'm testing this change right now and it's already looks promising.
As it's just a commit I needed to compile Godot from scratch, so not sure if everything will work 100% but I hope all good.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect small tests to work but I don’t think anyone in the godot developer community would be able to test the obscure browser combinations.

My vote is to send the branch into to a pre-release asap.

Edited:

Since @Faless is the web maintainer I don’t know who else can really review it.

@DjSapsan
Copy link

I tested it in my project. My application was finally loading data for a period but then started failing.
Sadly, I can't 100% confirm why it worked.
After it started failing I checked Wireshark and it's showing TLS 1.2 again. Not sure if it used 1.3 during successes.
No amount of reboots and code changes make it 1.3.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Linux, seems to work for me:

TLS 1.3 supported :)

Move library initialization to module registration functions.

Only set library debug threshold when verbose output is enabled.

TLSv1.3 functions seems to be a bit more verbose then expected, and
generate a lot of noise. Yet, some level of debugging without
recompiling the engine would be nice. We should discuss this upstream.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch to update the docs too.

@akien-mga akien-mga merged commit 35459bd into godotengine:master Sep 26, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@DjSapsan
Copy link

Testing again (Kubuntu btw).
It's showing the correct TLS 1.3, though I still have problems with loading data.
After first packet it stops refreshing

@DjSapsan
Copy link

DjSapsan commented Sep 26, 2024

I tested the issue deeper with Wireshark. I need to figure out if this problem with my code or the Godot's code.
I removed everything from my code and just receiving packets without any computation.
After a short working period it shows errors of ZeroWindow and sometimes others. It means that the client (Godot app) can't receive data fast enough. The Browser and Postman still loading the same data perfectly fine.
I think it has something to do with the buffers or maybe something else.
P.S. for understanding - in this case server sends a LOT of data in one burst and then small periodic updates. That's why it may not appear during small tests.
image

@Faless
Copy link
Collaborator Author

Faless commented Oct 4, 2024

@DjSapsan please open a dedicated issue with a minimal reproduction project. It's not really possible to understand what's going on otherwise.

@DjSapsan
Copy link

DjSapsan commented Oct 6, 2024

@DjSapsan please open a dedicated issue with a minimal reproduction project. It's not really possible to understand what's going on otherwise.

#97899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebSocketPeer uses TLSv1.2 and can't change to TLSv1.3
5 participants