Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x #82282

lyuma · 2023-09-25T04:13:09Z

Needed for libdatachannel support. Code compatibility was fairly straightforward. The only complexity was that a PRNG was required for parsing keys in CryptoKeyMbedTLS. Otherwise only a couple ifdefs were needed to keep compatibility with both mbedtls 3.x and 2.x.

In a non-rigorous test including both this and the libdatachannel integration, binary size was 1.1MB larger. I determined this size change was entirely due to libdatachannel.
I would like to break this down to see how much of this is from the mbedtls upgrade. Maybe we can compare github actions for a more accurate answer on binary size.

akien-mga · 2023-09-25T06:51:41Z

I would like to break this down to see how much of this is from the mbedtls upgrade. Maybe we can compare github actions for a more accurate answer on binary size.

I did a quick comparison with the artifacts from this PR and from https://github.com/godotengine/godot/actions/runs/6292543678 merged yesterday.

The current master is the folders without suffix, and this PR is the folders with (1) suffix.

161915061       linux-editor-mono
161947764       linux-editor-mono(1)
71153264        linux-template-mono
71173680        linux-template-mono(1)
36410618        web-template
36373175        web-template(1)
114429440       windows-editor
114424832       windows-editor(1)

So there's +20/30 kB for Linux editor/templates, but -40 kB for the Web template and the Windows editor. So it's pretty much an insignificant difference.

modules/mbedtls/SCsub

fire · 2023-10-05T19:28:20Z

@Faless isn't confident bumping the mbedtls version right now. Also, mbedTLS 3.5 just came out, so we should do the switch soon after the release to have an entire release cycle to ensure Godot Engine networking works. Switching to mbedTLS 3.x means more maintenance burden and potentially more instability.

The plan is to merge the mbedtls update after the Godot Engine 4.2 release is out.

akien-mga · 2023-10-05T19:39:32Z

Sounds like a good plan 👍

fire · 2024-01-08T22:05:57Z

@Faless The window for 4.3 is running down, what is the revised plan for this?

I don't see a mbedtls upgrade being essential if the webrtc isn't desired in the core, and that was the reason for the upgrade.

So the mbedlts upgrade is salvageable for someone?

Good reasons to salvage:

Prevent version drift
Support newer CryptoKey algorithms

lyuma · 2024-01-09T05:08:56Z

@fire You asked about being salvageable. I was able to rebase to latest master with no issue, so I think this PR should still be fine.

What are the next steps?

akien-mga · 2024-01-09T08:36:29Z

thirdparty/mbedtls/LICENSE

+===============================================================================
+
+
+                    GNU GENERAL PUBLIC LICENSE


We shouldn't include the text of the GPL, this will freak people out. The upstream Apache-licensed source tarball usually has the edited version of the LICENSE with only Apache (basically the file we already had).

akien-mga

I need to test a build against a distro mbedtls 2.x to confirm it's working, but I support moving to mbedtls 3 as our bundled version.

Reserving merge for approval by @Faless.

Faless

Great work 🏆 .

This looks overall good, see my suggestion about seeding rngs on-the-fly when parsing private keys.

Also, I noticed that building with module_mbedtls_enabled=no is giving some weird linking error:

$ scons module_mbedtls_enabled=no
scons: Reading SConscript files ...
Automatically detected platform: linuxbsd
Auto-detected 8 CPU cores available for build parallelism. Using 7 cores by default. You can override it with the -j argument.
Using linker program: gold
Building for platform "linuxbsd", architecture "x86_64", target "editor".
NOTE: Developer build, with debug optimization level and debug symbols (unless overridden).
Checking for C header file mntent.h... (cached) yes
scons: done reading SConscript files.
scons: Building targets ...
[ 96%] Linking Program bin/godot.linuxbsd.editor.dev.x86_64 ...
[100%] progress_finish(["progress_finish"], [])
[100%] thirdparty/mbedtls/library/common.h:221: error: undefined reference to 'mbedtls_get_unaligned_uint64'
thirdparty/mbedtls/library/common.h:221: error: undefined reference to 'mbedtls_get_unaligned_uint64'
thirdparty/mbedtls/library/common.h:222: error: undefined reference to 'mbedtls_put_unaligned_uint64'
thirdparty/mbedtls/library/aes.c:650: error: undefined reference to 'mbedtls_get_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:930: error: undefined reference to 'mbedtls_get_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:931: error: undefined reference to 'mbedtls_get_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:932: error: undefined reference to 'mbedtls_get_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:966: error: undefined reference to 'mbedtls_put_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:967: error: undefined reference to 'mbedtls_put_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:968: error: undefined reference to 'mbedtls_put_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:969: error: undefined reference to 'mbedtls_put_unaligned_uint32'
thirdparty/mbedtls/library/aes.c:1206: error: undefined reference to 'mbedtls_get_unaligned_uint64'
thirdparty/mbedtls/library/aes.c:1207: error: undefined reference to 'mbedtls_get_unaligned_uint64'
thirdparty/mbedtls/library/aes.c:1212: error: undefined reference to 'mbedtls_put_unaligned_uint64'
thirdparty/mbedtls/library/aes.c:1213: error: undefined reference to 'mbedtls_put_unaligned_uint64'
thirdparty/mbedtls/library/aes.c:1271: error: undefined reference to 'mbedtls_xor'
thirdparty/mbedtls/library/aes.c:1278: error: undefined reference to 'mbedtls_xor'
thirdparty/mbedtls/library/aes.c:1304: error: undefined reference to 'mbedtls_xor'
thirdparty/mbedtls/library/aes.c:1308: error: undefined reference to 'mbedtls_xor'
thirdparty/mbedtls/library/constant_time.c:213: error: undefined reference to 'mbedtls_put_unaligned_uint64'
collect2: error: ld returned 1 exit status
scons: *** [bin/godot.linuxbsd.editor.dev.x86_64] Error 1
scons: building terminated because of errors.
[Time elapsed: 00:00:28.522]

Faless · 2024-01-18T15:59:51Z

modules/mbedtls/crypto_mbedtls.cpp

+#if MBEDTLS_VERSION_MAJOR >= 3
+static mbedtls_entropy_context rng_entropy;
+static mbedtls_ctr_drbg_context rng_drbg;
+#endif


I'm a bit worried about this.

mbedTLS doesn't guarantee thread safety in some circumstances (e.g. the threading module can be disabled).

In similar cases (e.g. Crytpo/TLSContext), we always use per-instance rngs.

In this case, given the type of resource in question (CryptoKey) I suggest we instead create it on the fly when parsing.

diff --git a/modules/mbedtls/crypto_mbedtls.cpp b/modules/mbedtls/crypto_mbedtls.cpp index 4bb29d3b00..fbf48ea1e0 100644 --- a/modules/mbedtls/crypto_mbedtls.cpp +++ b/modules/mbedtls/crypto_mbedtls.cpp @@ -49,11 +49,6 @@ #define PEM_END_CRT "-----END CERTIFICATE-----\n" #define PEM_MIN_SIZE 54 -#if MBEDTLS_VERSION_MAJOR >= 3 -static mbedtls_entropy_context rng_entropy; -static mbedtls_ctr_drbg_context rng_drbg; -#endif - CryptoKey *CryptoKeyMbedTLS::create() { return memnew(CryptoKeyMbedTLS); } @@ -74,11 +69,7 @@ Error CryptoKeyMbedTLS::load(String p_path, bool p_public_only) { if (p_public_only) { ret = mbedtls_pk_parse_public_key(&pkey, out.ptr(), out.size()); } else { -#if MBEDTLS_VERSION_MAJOR >= 3 - ret = mbedtls_pk_parse_key(&pkey, out.ptr(), out.size(), nullptr, 0, mbedtls_ctr_drbg_random, &rng_drbg); -#else - ret = mbedtls_pk_parse_key(&pkey, out.ptr(), out.size(), nullptr, 0); -#endif + ret = _parse_key(out.ptr(), out.size()); } // We MUST zeroize the memory for safety! mbedtls_platform_zeroize(out.ptrw(), out.size()); @@ -117,11 +108,7 @@ Error CryptoKeyMbedTLS::load_from_string(String p_string_key, bool p_public_only if (p_public_only) { ret = mbedtls_pk_parse_public_key(&pkey, (unsigned char *)p_string_key.utf8().get_data(), p_string_key.utf8().size()); } else { -#if MBEDTLS_VERSION_MAJOR >= 3 - ret = mbedtls_pk_parse_key(&pkey, (unsigned char *)p_string_key.utf8().get_data(), p_string_key.utf8().size(), nullptr, 0, mbedtls_ctr_drbg_random, &rng_drbg); -#else - ret = mbedtls_pk_parse_key(&pkey, (unsigned char *)p_string_key.utf8().get_data(), p_string_key.utf8().size(), nullptr, 0); -#endif + ret = _parse_key((unsigned char *)p_string_key.utf8().get_data(), p_string_key.utf8().size()); } ERR_FAIL_COND_V_MSG(ret, FAILED, "Error parsing key '" + itos(ret) + "'."); @@ -147,6 +134,25 @@ String CryptoKeyMbedTLS::save_to_string(bool p_public_only) { return s; } +int CryptoKeyMbedTLS::_parse_key(const uint8_t *p_buf, int p_size) { +#if MBEDTLS_VERSION_MAJOR >= 3 + mbedtls_entropy_context rng_entropy; + mbedtls_ctr_drbg_context rng_drbg; + + mbedtls_ctr_drbg_init(&rng_drbg); + mbedtls_entropy_init(&rng_entropy); + int ret = mbedtls_ctr_drbg_seed(&rng_drbg, mbedtls_entropy_func, &rng_entropy, nullptr, 0); + ERR_FAIL_COND_V_MSG(ret != 0, ret, vformat("mbedtls_ctr_drbg_seed returned -0x%x\n", (unsigned int)-ret)); + + ret = mbedtls_pk_parse_key(&pkey, p_buf, p_size, nullptr, 0, mbedtls_ctr_drbg_random, &rng_drbg); + mbedtls_ctr_drbg_free(&rng_drbg); + mbedtls_entropy_free(&rng_entropy); + return ret; +#else + return mbedtls_pk_parse_key(&pkey, p_buf, p_size, nullptr, 0); +#endif +} + X509Certificate *X509CertificateMbedTLS::create() { return memnew(X509CertificateMbedTLS); } @@ -314,17 +320,6 @@ void CryptoMbedTLS::initialize_crypto() { Crypto::_create = create; Crypto::_load_default_certificates = load_default_certificates; - -#if MBEDTLS_VERSION_MAJOR >= 3 - mbedtls_ctr_drbg_init(&rng_drbg); - mbedtls_entropy_init(&rng_entropy); - const char *pers = "godot personalize"; - int ret = mbedtls_ctr_drbg_seed(&rng_drbg, mbedtls_entropy_func, &rng_entropy, (const unsigned char *)pers, strlen(pers)); - if (ret != 0) { - ERR_PRINT(vformat("mbedtls_ctr_drbg_seed returned -0x%x\n", (unsigned int)-ret)); - } -#endif - X509CertificateMbedTLS::make_default(); CryptoKeyMbedTLS::make_default(); HMACContextMbedTLS::make_default(); @@ -340,10 +335,6 @@ void CryptoMbedTLS::finalize_crypto() { X509CertificateMbedTLS::finalize(); CryptoKeyMbedTLS::finalize(); HMACContextMbedTLS::finalize(); -#if MBEDTLS_VERSION_MAJOR >= 3 - mbedtls_ctr_drbg_free(&rng_drbg); - mbedtls_entropy_free(&rng_entropy); -#endif } CryptoMbedTLS::CryptoMbedTLS() { diff --git a/modules/mbedtls/crypto_mbedtls.h b/modules/mbedtls/crypto_mbedtls.h index 0168e1f663..18f0dadb9d 100644 --- a/modules/mbedtls/crypto_mbedtls.h +++ b/modules/mbedtls/crypto_mbedtls.h @@ -46,6 +46,8 @@ private: int locks = 0; bool public_only = true; + int _parse_key(const uint8_t *p_buf, int p_size); + public: static CryptoKey *create(); static void make_default() { CryptoKey::_create = create; }

fire · 2024-04-04T19:34:54Z

I did some work on mbedtls 3.6 upgrade in the webrtc branch but it's relatively hard to work with. Needs help.

https://github.com/V-Sekai/godot/tree/vsk-libdatachannel-4.3

akien-mga · 2024-04-10T11:28:10Z

Superseded by #90482.

lyuma requested review from a team as code owners September 25, 2023 04:13

lyuma force-pushed the mbedtls branch from 02aecb8 to ad18cae Compare September 25, 2023 04:24

lyuma mentioned this pull request Sep 25, 2023

Integrate libdatachannel 0.19.1 in the core webrtc module. #82284

Open

akien-mga added enhancement topic:buildsystem topic:thirdparty labels Sep 25, 2023

akien-mga added this to the 4.2 milestone Sep 25, 2023

akien-mga removed the topic:buildsystem label Sep 25, 2023

akien-mga reviewed Sep 25, 2023

View reviewed changes

modules/mbedtls/SCsub Outdated Show resolved Hide resolved

lyuma force-pushed the mbedtls branch 2 times, most recently from 202da6f to 05e1efd Compare September 25, 2023 20:56

akien-mga modified the milestones: 4.2, 4.3 Oct 5, 2023

fire force-pushed the mbedtls branch from 05e1efd to 202b703 Compare November 17, 2023 03:42

fire changed the title ~~Update builtin mbedtls to 3.4.1, keeping compatiblity with mbedtls 2.x~~ Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x Nov 17, 2023

fire force-pushed the mbedtls branch 4 times, most recently from fd0ba0b to d791a54 Compare November 17, 2023 04:26

Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x

8a7658e

lyuma force-pushed the mbedtls branch from d791a54 to 8a7658e Compare January 9, 2024 05:05

akien-mga reviewed Jan 9, 2024

View reviewed changes

akien-mga approved these changes Jan 9, 2024

View reviewed changes

Faless requested changes Jan 18, 2024

View reviewed changes

akien-mga mentioned this pull request Apr 4, 2024

mbedtls: Update to upstream version 2.28.8 #90209

Merged

Faless mentioned this pull request Apr 10, 2024

mbedTLS: Update to new LTS v3.6.0 #90482

Merged

akien-mga closed this Apr 10, 2024

akien-mga added the archived label Apr 10, 2024

AThousandShips removed this from the 4.3 milestone Apr 10, 2024

fire deleted the mbedtls branch April 10, 2024 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x #82282

Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x #82282

lyuma commented Sep 25, 2023 •

edited

Loading

akien-mga commented Sep 25, 2023 •

edited

Loading

fire commented Oct 5, 2023

akien-mga commented Oct 5, 2023

fire commented Jan 8, 2024

lyuma commented Jan 9, 2024

akien-mga Jan 9, 2024

akien-mga left a comment

Faless left a comment

Faless Jan 18, 2024

fire commented Apr 4, 2024

akien-mga commented Apr 10, 2024

		===============================================================================


		GNU GENERAL PUBLIC LICENSE

Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x #82282

Update builtin mbedtls to 3.5.1, keeping compatiblity with mbedtls 2.x #82282

Conversation

lyuma commented Sep 25, 2023 • edited Loading

akien-mga commented Sep 25, 2023 • edited Loading

fire commented Oct 5, 2023

akien-mga commented Oct 5, 2023

fire commented Jan 8, 2024

lyuma commented Jan 9, 2024

akien-mga Jan 9, 2024

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

Faless left a comment

Choose a reason for hiding this comment

Faless Jan 18, 2024

Choose a reason for hiding this comment

fire commented Apr 4, 2024

akien-mga commented Apr 10, 2024

lyuma commented Sep 25, 2023 •

edited

Loading

akien-mga commented Sep 25, 2023 •

edited

Loading