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

[WIP - RFC] Initial work Crypto, SSL server, crt/key as Resource #29871

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Faless
Copy link
Contributor

commented Jun 18, 2019

This PR includes:

  • New hex_encode function for PoolByteArray that convert the bytes to an hex string.
  • New Crypto class that can generate random bytes, rsa keys, self signed certificates.
  • Certificates (crt) and Keys (key) are considered Resource. You can load and save them.
  • Implemented StreamPeerSSL.accept_stream (SSL server) and allow selecting specific valid CAs when calling conntect_to_stream.
  • AES ECB encryption via the AESContext class (currently broken, working on a fix).
  • MD5/SHA1/SHA256 hashing via the HashingContext class.

This is also some ground work towards implementing DTLS, and internal HTTP(S) server for HTML5 debug.

Some interesting points:

  • Certs and keys gets "locked" when in use by a stream, and can't be changed when in use.
    This is so you can have a single instance of key/cert, and use it across multiple peers, without holding.
  • Should we have a default "Crypto" singleton (right now it's an object you instance)? I like having the possibility of them having different seeds, but having a default one would be nice.
  • Should we use the same random number generator across all SSL sessions (could that be a weakness?), right now it uses one rng per session as before.

A small example that uses this PR (SSL server with self-signed certificate, visit https://localhost:4343 after opening):

extends Node

class Client:
	var ssl = StreamPeerSSL.new()
	var recv = ""

	func set_stream(stream, key, cert):
		ssl.blocking_handshake = false
		ssl.accept_stream(stream, key, cert)

	func process():
		if ssl.get_status() != StreamPeerSSL.STATUS_CONNECTED:
			return
		ssl.poll()
		if ssl.get_available_bytes() > 0:
			recv += ssl.get_data(ssl.get_available_bytes())[1].get_string_from_utf8()
		if recv.ends_with("\r\n\r\n"):
			ssl.put_data(("HTTP/1.0 200 OK\r\nContent-Type: text/html\r\n\r\n" + \
						"<h2>Godot TLS Test Server</h2>\r\n"  + \
						"<p>Successful connection using SSL</p>\r\n").to_utf8())
			ssl.disconnect_from_stream()

var _server = TCP_Server.new()
var _clients = []
var _cert = null
var _key = null

func _ready():
	var crypto = Crypto.new()
	_key = crypto.generate_rsa(4096)
	_cert = crypto.generate_self_signed_certificate(_key, "CN=aserver,O=A Game Company,C=IT")
	_key.save("res://generated.key")
	_cert.save("res://generated.crt")
	_server.listen(4343)

func _process(delta):
	if _server.is_connection_available():
		var c = Client.new()
		c.set_stream(_server.take_connection(), _key, _cert)
		_clients.append(c)

	for c in _clients:
		c.process()

Another example showing hahsing:

extends Node

func _ready():
	var ctx = HashingContext.new()
	ctx.start(HashingContext.HASH_MD5) # Or HASH_SHA1 or HASH_SHA256
	print("a".to_utf8().size())
	ctx.update("a".to_ascii())
	var fin = ctx.finish()
	print(Array(fin))
	print(fin.hex_encode())

Fixes #27045 .

References (possibly close after some more work): #29871 #25928 #1969

@Faless Faless added this to the 3.2 milestone Jun 18, 2019

@Faless Faless requested a review from godotengine/network as a code owner Jun 18, 2019

@nate066

This comment has been minimized.

Copy link

commented Jun 19, 2019

This looks really cool, I hope AES support will happen along with a hashing algorithm like argon2. :)

@fire

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

How do you want this reviewed? Is there anything in particular you want to look for? Did you make basic demos that use these and do they run as expected?

@fire

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

Also, can you improve the quality of the commit messages and reduce the number of commits?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

How do you want this reviewed?

@fire It's still WIP, so mainly in terms of structure (see interesting points)

Did you make basic demos that use these and do they run as expected?

Just the attached script, and yes, it does run correctly

Also, can you improve the quality of the commit messages and reduce the number of commits?

Yes, I will, before removing the WIP tag, but it's likely going to be 2 commits in the end.

  • Adding crypto
    (modules/mbdetls/crypto_mbedtls.* and core/math/crypto.*)
  • Porting StreamPeerSSL to crypto
    (core/io/stream_peer_ssl.*, modules/mbdetls/ssl_context_mbed_tls.*, modules/mbdetls/stream_peer_mbedtls.*)
@aspenforest

This comment has been minimized.

Copy link

commented Jun 23, 2019

Please consider adding SHA-3 and BLAKE hash functions and Fortuna CS-RNG. :)

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

FYI, the javascript platform currently disables the mbedTLS module with this comment:

        # Disabling the mbedtls module reduces file size.
        # The module has little use due to the limited networking functionality
        # in this platform. For the available networking methods, the browser
        # manages TLS.
        ('module_mbedtls_enabled', False),

I guess we might want to enable it again for this module?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

I guess we might want to enable it again for this module?

Yeah, an alternative is for Javascript to have its own implementation via the CryptoAPI.

But I would prefer having only one implementation for ease of maintenance, it should pretty much work out of the box, the only difference might be in the entropy function.

I'll do a size comparison with and without the module to see how much bigger we get in the HTML5 export.

@fire

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Are you blocked on anything? Is there anything we can do to help you?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Are you blocked on anything? Is there anything we can do to help you?

I've been busy on the WebSocket library switch and some other fixes.
I'm finally back to this, so you can expect an update soon, I'm thinking of adding AES and support to streamed hashing before marking this as ready for review :)

@fire

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Can you split those changes to another PR, or do they require code stubs for them?

I wish to try this in master without custom merging this branch.

Faless added some commits Jul 11, 2019

Fix CryptoCore signatures, add SHA1 context.
Fix hash size in SHA256 signature
Fix source parameter in hash context update function to be const.
Add SHA1 hash context.
Move CryptoCore to it's own folder.
Crypto classes will be placed in core/crypto.

@Faless Faless force-pushed the Faless:crypto/initial_pr branch from 37c9bbe to e18ea11 Jul 19, 2019

@Faless Faless requested a review from vnen as a code owner Jul 19, 2019

@Faless Faless force-pushed the Faless:crypto/initial_pr branch from e18ea11 to 28b1a7c Jul 19, 2019

Faless added some commits Jul 19, 2019

New CryptoMbedTLS Crypto implementation.
Allows random bytes, RSA keys, and X509 certificates generation.
Rewrite StreamPeerSSL with SSLContext helper class
connect_to_stream now accepts optional parameter to specify which
certificates to trust.
Implement accept_stream (SSL server) with key/cert parameters to specify
the RSA key and X509 certificate resources.

@Faless Faless force-pushed the Faless:crypto/initial_pr branch from 28b1a7c to b53f8e7 Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.