yuzu icon indicating copy to clipboard operation
yuzu copied to clipboard

build: use system MbedTLS when available, switch to upstream 2.28.1

Open Tachi107 opened this issue 2 years ago • 34 comments

Since MbedTLS pre-3.0.0 doesn't ship neither a pkg-config file nor a CMake package config file it is required to use a custom FindMbedTLS.cmake file.

Since yuzu requires CMAC support it is also needed to check for the mbedtls_cipher_cmac symbol.

To avoid API incompatibilities, find_package() only accepts version 2.x.

I also changed src/core/CMakeLists.txt to only link against mbedcrypto, as yuzu doesn't use the full MbedTLS library

Tachi107 avatar Sep 18 '21 13:09 Tachi107

Have we confirmed that 3.0 remains API compatible with the functions we use? I'd prefer that we migrate to 3.0 for all platforms when an LTS is available.

Morph1984 avatar Sep 18 '21 14:09 Morph1984

According to ARMmbed/mbedtls/docs/3.0-migration-guide.md the function mbedtls_sha256_ret has been removed, and it is used by yuzu in various places.

https://github.com/yuzu-emu/yuzu/blob/1b09d6628b7f268c256c12e4d3e373724671d2f1/src/core/file_sys/registered_cache.cpp#L67

https://github.com/yuzu-emu/yuzu/blob/1b09d6628b7f268c256c12e4d3e373724671d2f1/src/core/file_sys/registered_cache.cpp#L149

https://github.com/yuzu-emu/yuzu/blob/065867e2c24e9856c360fc2d6b9a86c92aedc43e/src/core/file_sys/xts_archive.cpp#L69

https://github.com/yuzu-emu/yuzu/blob/c5ca8675c84ca73375cf3fe2ade257c8aa5c1239/src/core/crypto/partition_data_manager.cpp#L183

https://github.com/yuzu-emu/yuzu/blob/c5ca8675c84ca73375cf3fe2ade257c8aa5c1239/src/core/crypto/partition_data_manager.cpp#L211

https://github.com/yuzu-emu/yuzu/blob/00ce8eff65190d2049e25665fe0b3f99f716a99f/src/core/hle/service/bcat/backend/boxcat.cpp#L280

https://github.com/yuzu-emu/yuzu/blob/4ea171fa5e50723d50b57b26aaaca2fac30eea57/src/core/hle/service/ldr/ldr.cpp#L472

https://github.com/yuzu-emu/yuzu/blob/1b09d6628b7f268c256c12e4d3e373724671d2f1/src/core/crypto/key_manager.cpp#L488

Tachi107 avatar Sep 18 '21 15:09 Tachi107

Also, other than API compatibility, is there a benefit to using system mbedtls when available? This would introduce version inconsistencies between distributions

Morph1984 avatar Sep 18 '21 15:09 Morph1984

(slightly) Faster compile times, smaller binary size, possibilty to update MbedTLS without having to recompile yuzu, and would make things simpler for downstream. This is also for consistency, since a lot of other dependencies are used from the system when available. Unless yuzu has to depend on a particular version for a specific reason (this does not seem the case, but correct me if I'm wrong) I see no downsides to using already installed libraries (when they are API-compatible, obviously)

Tachi107 avatar Sep 18 '21 15:09 Tachi107

The major blocker here would be API compatibility (obviously, given that 3.0 has just released), and we've had issues in the past where system bundled libraries are severely out of date / lack the features we use (since we tend to use very new features from certain libraries), so we set a minimum version for all build targets, use system libraries when they meet the minimum version we use, and fallback to externals when they dont meet this requirement.

Morph1984 avatar Sep 18 '21 15:09 Morph1984

Also, it seems that only LTS versions of mbedtls are licensed under GPLv2 or later, non-LTS releases are only licensed under the Apache 2.0 license

Morph1984 avatar Sep 18 '21 15:09 Morph1984

The major blocker here would be API compatibility (obviously, given that 3.0 has just released), and we've had issues in the past where system bundled libraries are severely out of date / lack the features we use (since we tend to use very new features from certain libraries), so we set a minimum version for all build targets, use system libraries when they meet the minimum version we use, and fallback to externals when they dont meet this requirement.

Yes, in fact in this FindMbedTLS.cmake file I check for the mbedtls_cipher_cmac, since CMAC support is not enabled by default in MbedTLS (and this works; for example, in the Debian package of the library the symbol isn't present, and yuzu correctly falls back to using the one from externals). Since in 3.0 mbedtls_sha256_ret has been removed it would be appropriate to also check for that symbol.

Also, it seems that only LTS versions of mbedtls are licensed under GPLv2 or later, non-LTS releases are only licensed under the Apache 2.0 license

That's right, yuzu can't switch to 3.x yet.

Edit: I don't know if this is clear, but with this PR I'm not suggesting to switch to 3.0, I just mentioned it for completeness

Edit 2: The list of GPL-licensed MbedTLS versions is available here: https://tls.mbed.org/download-archive

Tachi107 avatar Sep 18 '21 15:09 Tachi107

Edit: I don't know if this is clear, but with this PR I'm not suggesting to switch to 3.0, I just mentioned it for completeness

Yes, I am aware, but I am also accounting for the scenario where some distributions may include API breaking versions (newer or older, depending on if yuzu uses 2.x or 3.x)

Edit 2: The list of GPL-licensed MbedTLS versions is available here: https://tls.mbed.org/download-archive

Yes, this is why we use 2.16 in our externals

Morph1984 avatar Sep 18 '21 15:09 Morph1984

Most of the libraries where we use the system library instead of externals have relativey stable APIs, or/and provide us a method of easily checking the versions without having to test for features

Morph1984 avatar Sep 18 '21 16:09 Morph1984

I am also accounting for the scenario where some distributions may include API breaking versions (newer or older, depending on if yuzu uses 2.x or 3.x)

Easy solution! mbedtls/version.h contains MBEDTLS_VERSION_MAJOR and MBEDTLS_VERSION_MINOR, and the custom FindMbedTLS.cmake can check for the version number to ensure that the found version is compatible (just checking the major version is sufficient, since MbedTLS does not introduce API changes between minor versions, but the check could be more strict if needed, checking for a specific LTS version).

Again, the package config file in 3.x will allow to check for the required version automatically (find_package(MbedTLS 3.lts))

Tachi107 avatar Sep 18 '21 16:09 Tachi107

There's also one other detail I've just recalled. When I've migrsted the project from an older version mbedtls 2.x to 2.16, there was a small API breaking change due to security fixes, so even in minor version releases API compatibilty isn't 100% guaranteed (mbedtls themselves have said this)

Morph1984 avatar Sep 18 '21 16:09 Morph1984

There's also one other detail I've just recalled. When I've migrsted the project from an older version mbedtls 2.x to 2.16, there was a small API breaking change due to security fixes, so even in minor version releases API compatibilty isn't 100% guaranteed (mbedtls themselves have said this)

Perfect, I'll check for both version numbers

Tachi107 avatar Sep 18 '21 16:09 Tachi107

Now find_package() will only accept MbedTLS version 2.16.x :)

Edit: Ops, fixing now...

Edit 2: Fixed

Tachi107 avatar Sep 18 '21 21:09 Tachi107

Hey @Morph1984, CMAC support in libmbedcrypto has been recently added in Debian (see here), so this patch will be now useful in practice for Debian (and derivatives like Ubuntu) users. What's preventing this from being merged?

Tachi107 avatar Nov 20 '21 14:11 Tachi107

I think this can be cleaned up a bit more before merging. Converting to draft.

Tachi107 avatar Dec 10 '21 19:12 Tachi107

Cleaned up a bit, ready for review :)

Tachi107 avatar Dec 11 '21 18:12 Tachi107

@Tachi107 any chance you can make this work with 2.28.0?

Tatsh avatar Dec 26 '21 22:12 Tatsh

@Tatsh sure, I think that this should already work without changes, simply updating ​find_package​(MbedTLS 2.16 ​EXACT​) to 2.28. I can't test this now though

Tachi107 avatar Dec 27 '21 10:12 Tachi107

Could you make this have a flag to fail if system mbedtls is not found rather than simply falling back?

There are other flags in the system like YUZU_USE_BUNDLED_X so that YUZU_USE_BUNDLED_X=NO will cause the configuration to fail when X is not found.

Tatsh avatar Dec 27 '21 18:12 Tatsh

Could you make this have a flag to fail if system mbedtls is not found rather than simply falling back?

I personally dislike having too many options, especially when they are not needed. Why do you want the build to fail if system MbedTLS is not found? For distro packaging needed build dependencies are always present, and end users compiling yuzu on their own don't/shouldn't really care if they are using the system version of MbedTLS or the bundled one.

An option may be desirable if using the system version could cause issues, performance degradation, etc, but for trivial usages like this a dedicated option is not needed. But I may be missing something here, so please prove me wrong :)

Edit: also, every new line of code means more things to maintain

Edit 2: the "use system deps if available, fall back to conan/bundled" approach is what's used for the vast majority of dependencies in yuzu, so this is also for consistency

Tachi107 avatar Dec 27 '21 20:12 Tachi107

I'd like to note that the last version of MbedTLS that was dual-licensed under the Apache-2.0 OR GPL-2.0-or-later has reached end-of-life with the end of 2021, and newer versions are only licensed under the Apache-2.0. Even if yuzu's license, the GPL-2.0-or-later, is incompatible with the Apache-2.0 I believe that it is not an issue, since yuzu and MbedTLS are two separate programs and MbedTLS is not a derivative work of the emulator, since it simply links to it (but I'm not a lawyer, so I may be wrong about this important detail).

One thing that I am not sure about is whether or not yuzu is allowed to embed Apache-2.0 code in its repository; if not, the only way yuzu would be able to use updated versions of MbedTLS would be to link to an externally built version of it, and this repo would be forced to keep its embedded version of the library at version 2.16.12 or older.

Tachi107 avatar Jan 12 '22 17:01 Tachi107

As yuzu is now distributed under the GPL-3.0-or-later, MbedTLS 2.28 and newer can be now used without issues 🎉️

Tachi107 avatar Mar 26 '22 14:03 Tachi107

Hey @Morph1984, I've now also updated this PR to use upstream's MbedTLS in externals instead of using your own fork in yuzu-emu/MbedTLS, as your UTF-8 fix is now included in the latest LTS release, 2.28.1.

Could you please tell me if this PR is ready to be merged? Maintaining out of tree patches is kinda annoying. Thanks!

Tachi107 avatar Jul 22 '22 15:07 Tachi107

Rebased again, and solved the remaining issues. This is finally ready to be merged (again) :)

Tachi107 avatar Jul 27 '22 14:07 Tachi107

Since we're now GPLv3, can we move to mbedtls 3.2?

Morph1984 avatar Jul 27 '22 15:07 Morph1984

You could, but I wouldn't. MbedTLS 3 still doesn't have an LTS branch

Tachi107 avatar Jul 27 '22 15:07 Tachi107

I'm planning to move to 3.2.1 in an upcoming rewrite, since an LTS would still be a couple years away. At this point I'd rather take the ABI break

Morph1984 avatar Jul 28 '22 00:07 Morph1984

That way you'd make it really hard for Linux distros to package yuzu, as they usually only package LTS, if available (I'm the Debian/Ubuntu MbedTLS maintainer, and that's what I do).

If there's a good reason to upgrade (amazing new API, new features that you absolutely need, big performance improvements, etc.) go ahead, of course. But if it is just to follow upstream's latest release, please don't :pleading_face:

Tachi107 avatar Jul 28 '22 10:07 Tachi107

The primary reasons for the move to 3.x as opposed to the 2.28 series are the API changes to RSA encryption and upcoming SHA-3 support

Morph1984 avatar Jul 28 '22 10:07 Morph1984

But MbedTLS 3.2 doesn't support SHA-3 yet, right? In that case, why not waiting for a release that includes SHA-3 support before the switch to the 3.x branch? That would give me more time to figure out how to handle this.

Who knows, maybe the next release to include SHA-3 support will also be an LTS one :)

Tachi107 avatar Jul 28 '22 10:07 Tachi107