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

CryptoCore class to access to base crypto utils. #30239

Merged
merged 1 commit into from Jul 2, 2019

Conversation

@Faless
Copy link
Contributor

commented Jul 2, 2019

Godot core needs MD5/SHA256/AES/Base64 which used to be provided by
separate libraries.
Since we bundle mbedtls in most cases, and we can easily only include
the needed sources if we so desire, let's use it.

To simplify library changes in the future, and better isolate header
dependencies all functions have been wrapped around inside a class in
core/math/crypto_base.h.

If the mbedtls module is disabled, we only bundle the needed source
files independently of the builtin_mbedtls option.
If the module is enabled, the builtin_mbedtls option works as usual.

Also remove some unused headers from StreamPeerMbedTLS which were
causing build issues.

@Faless Faless added this to the 3.2 milestone Jul 2, 2019
@Faless Faless requested review from akien-mga, vnen and godotengine/network as code owners Jul 2, 2019
core/math/crypto_core.cpp Outdated Show resolved Hide resolved
core/math/SCsub Show resolved Hide resolved
@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

I guess it's called CryptoCore because you intend to add a non-core Crypto module eventually?

@Faless Faless force-pushed the Faless:crypto/crypto_core branch from 716943f to 66f5c6c Jul 2, 2019
@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

I guess it's called CryptoCore because you intend to add a non-core Crypto module eventually?

Yeah, well, basically the work in #29871 .
The problem is that when the mbedtls module is not compiled, the "non-core" crypto class (exposed to GDScript in that PR) will not work (in that PR it's a custom instance class, i.e. the constructor will return NULL if the module is not available).

We could have a default dummy Crypto implementation that provide only the CryptoCore functions, but it's going to be a bit annoying to write and keep in sync all those empty functions. Opinions?

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Did some testing locally, this basic test passes fine before and after the PR (art file from Jetpaca repo):

var file = File.new()
assert(file.get_md5("res://art/intro_alpaca.png") == "02921cf3d7c07d8d17281c9577cf4d17")
assert(file.get_sha256("res://art/intro_alpaca.png") == "e3feb46f34974f7587b0e5db56a989c2a1c0ef0a7dba6f3b3dc46c1dc54edff5")
assert("Hello darkness, my old friend.".md5_text() == "a3e894985de5a5cc555a1d95af8b5572")
assert("Hello darkness, my old friend.".md5_buffer() == PoolByteArray([163, 232, 148, 152, 93, 229, 165, 204, 85, 90, 29, 149, 175, 139, 85, 114]))
assert("Hello darkness, my old friend.".sha256_text() == "1aad712f682a32170525a4e6f4f737a0fcda35d134a0473ac3486da20617939e")
assert("Hello darkness, my old friend.".sha256_buffer() == PoolByteArray([26, 173, 113, 47, 104, 42, 50, 23, 5, 37, 164, 230, 244, 247, 55, 160, 252, 218, 53, 209, 52, 160, 71, 58, 195, 72, 109, 162, 6, 23, 147, 158]))

I also checked that .import files don't change needlessly, so the md5 sums are the same, even when forcing a reimport after deleting the .import/ folder (hopefully! but it's better to check :P).

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

We could have a default dummy Crypto implementation that provide only the CryptoCore functions, but it's going to be a bit annoying to write and keep in sync all those empty functions. Opinions?

Hm yeah, that's not easy. It would indeed be best from an end user point of view that everything resides in a Crypto singleton, which would have only the CryptoCore methods if the mbedtls or crypto modules are disabled, and all the additional crypto module methods otherwise.

But I don't think we have any easy way right now to make a module that would extend a core API in this manner.

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

(hopefully! but it's better to check :P).

I also tried AES256 script encryption. Files can be encoded/decoded fine with the right key.
I haven't yet checked exporting from another branch and opening it with this branch. I'll give that a try too.

Other two ("unrelated") issues I found, are that with module_mbedtls_enabled=no the editor crash.

One is a easy fix (StreamPeerSSL::create() should return null if not available):

diff --git a/core/io/stream_peer_ssl.cpp b/core/io/stream_peer_ssl.cpp
index 254ae84bf..ccce48ccd 100644
--- a/core/io/stream_peer_ssl.cpp
+++ b/core/io/stream_peer_ssl.cpp
@@ -39,7 +39,9 @@ StreamPeerSSL *(*StreamPeerSSL::_create)() = NULL;
 
 StreamPeerSSL *StreamPeerSSL::create() {
 
-       return _create();
+       if (_create)
+               return _create();
+       return NULL;
 }
 
 StreamPeerSSL::LoadCertsFromMemory StreamPeerSSL::load_certs_func = NULL;

The other is more complex. Basically, the EditorNode do not load the asset library if SSL is not available (because our API rightly require SSL). The problem is that then a button is missing, but the EditorNode still uses is (EditorNode::EditorTable::EDITOR_ASSETLIB). Fixing that is a bit more tricky.
Should I open a separate PR?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

Should I open a separate PR?

Opened a issue about this: #30246

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Should I open a separate PR?

Fixing #30246 in a separate PR would be best yeah.

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

I haven't yet checked exporting from another branch and opening it with this branch. I'll give that a try too.

I confirm AES seems to work fine too. Both exporting from master, loading from branch, and exporting from branch and loading from master.

Copy link
Member

left a comment

Looks great to me. Needs confirmation from reduz that he's fine with the strong dependency added on (parts of) mbedTLS, but it seems a fair tradeoff for dropping those custom thirdparty files and have a better abstraction.

COPYRIGHT.txt Outdated Show resolved Hide resolved
Godot core needs MD5/SHA256/AES/Base64 which used to be provided by
separate libraries.
Since we bundle mbedtls in most cases, and we can easily only include
the needed sources if we so desire, let's use it.

To simplify library changes in the future, and better isolate header
dependencies all functions have been wrapped around inside a class in
`core/math/crypto_base.h`.

If the mbedtls module is disabled, we only bundle the needed source
files independently of the `builtin_mbedtls` option.
If the module is enabled, the `builtin_mbedtls` option works as usual.

Also remove some unused headers from StreamPeerMbedTLS which were
causing build issues.
@Faless Faless force-pushed the Faless:crypto/crypto_core branch from 66f5c6c to 564d93f Jul 2, 2019
@akien-mga akien-mga merged commit e9d624d into godotengine:master Jul 2, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Thanks!

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