godot icon indicating copy to clipboard operation
godot copied to clipboard

[NET] Refactor TLS configuration.

Open Faless opened this issue 1 year ago • 3 comments

Use a TLSOptions configuration object which is created via static functions.

  • "TLSOptions.client": uses the standard CA and common name verification.
  • "TLSOptions.client_unsafe": uses optional CA verification (i.e. if specified)
  • "TLSOptions.server": is the standard server configuration (chain + key)

This will allow us to expand the TLS configuration options to include e.g. mutual authentication without bloating the classes that uses StreamPeerTLS and PacketPeerDTLS as underlying peers.

This PR also removes the StreamPeerTLS.blocking_handshake member, since it's not used internally and can be replaced by:

while tls.get_status() == tls.STATUS_HANDSHAKING:
	tls.poll()

Faless avatar Jan 24 '23 20:01 Faless

GCC leak sanitizer seems to find a leak when running @qarmin's regression test project, though the output isn't clear as to where: https://github.com/godotengine/godot/actions/runs/4031300178/jobs/6930513991

akien-mga avatar Jan 28 '23 12:01 akien-mga

Latest line shows that problem is not with memory leak, but assertion in project

ERROR: Assertion failed in project, check execution log for more info

TLSOptions and Node3DGizmo are the only classes that can be used as function arguments, but they, as well as classes inheriting from them, do not have a default constructor(cannot be created with new() or ClassDB.instance())

I don't think it would be straightforward to add support for testing this type of classes, so for the time being they are ignored.

qarmin avatar Jan 28 '23 18:01 qarmin

To be clear, the test project was updated in https://github.com/godotengine/regression-test-project/commit/4fb7982ed34068fb9d7b27fc5e4b125c6312e883 so rebasing this PR should fix the CI.

akien-mga avatar Jan 29 '23 01:01 akien-mga

Only thought is that it's defined in core/crypto/crypto.h but only used in core/io/, so I wonder if it wouldn't make more sense there?

I think it's fine in crypto for now mostly because it is in practice a "struct" of X509Certificates and CryptoKeys which are also in core/crypto/crypto.h. We might want to refactor that header too but I'd rather do that in a separate PR in case.

Faless avatar Jan 30 '23 10:01 Faless

Thanks!

akien-mga avatar Jan 30 '23 12:01 akien-mga