HaxeFoundation / hxcpp

Runtime files for c++ backend for haxe
Other
295 stars 184 forks source link

mbedtls3 migration #1113

Open Aidan63 opened 3 months ago

Aidan63 commented 3 months ago

Mbedtls 2.28 goes EOL at the end of this year, so he's a switch over to 3.6.0 which was made LTS a few months ago. Only a few changes were needed to SSL.cpp, for the config file I just copied and pasted the default and change the few things needed.

There are no real ssl tests anywhere so I tested this by creating a quick ssl socket server and client and they were able to connect to non haxe client and servers fine so it seems to work.

Depending on when this gets merged in I'll be pulling these changes into my asys branch so I can build any tls stuff on top of the latest mbedtls version.

Aidan63 commented 3 months ago

@tobil4sk Do you know if the c99 flag is needed for mbedtls3? I'm not setup to develop for android but if it still needed I can add that flag to this merge so android will build again when either of these two get through (https://github.com/HaxeFoundation/hxcpp/pull/1111).

Aidan63 commented 2 months ago

Linux CI seems to be failing due to some unrelated mariadb setup issue. Mac and Windows are failing with lots of weird errors when compiling the cppia project.

D:/a/1/s/project/thirdparty/mbedtls-3.6.0/include\psa/crypto.h(110): error C2526: 'psa_key_attributes_init': C linkage function cannot return C++ class 'psa_key_attributes_s'
D:\a\1\s\project\thirdparty\mbedtls-3.6.0\include\psa\crypto_types.h(432): note: see declaration of 'psa_key_attributes_s'

D:\a\1\s\project\thirdparty\mbedtls-3.6.0\include\psa\crypto_extra.h(1874): error C2556: 'psa_pake_operation_s psa_pake_operation_init(void)': overloaded function differs only by return type from 'void psa_pake_operation_init(void)'
D:\a\1\s\project\thirdparty\mbedtls-3.6.0\include\psa\crypto_extra.h(1051): note: see declaration of 'psa_pake_operation_init'

D:\a\1\s\project\thirdparty\mbedtls-3.6.0\include\psa\crypto_extra.h(1873): error C2371: 'psa_pake_operation_init': redefinition; different basic types
D:\a\1\s\project\thirdparty\mbedtls-3.6.0\include\psa\crypto_extra.h(1051): note: see declaration of 'psa_pake_operation_init'

Annoyingly that cppia stage of the CI compiles absolutely fine on locally on my PC, so more investigation needed there...

Aidan63 commented 2 months ago

Ahh the joys of C++, the issue seems to be that mbedtls is written in c and uses some c features which were only added in C++20, GCC however has had extensions to add those features in C++ whereas clang and msvc do not.

https://github.com/Mbed-TLS/mbedtls/issues/7087

The visual studio / msvc I have installed must be newer than the CI image and have C++20 enabled by default for it to work fine on my machine. There's still no official fix but I applied one of the patches floating around from issues linked in the above and the Windows and Mac CI now work (mac ci failing due to existing flakey test).

tobil4sk commented 2 months ago

Do you know if the c99 flag is needed for mbedtls3?

Yes, mbedtls is written in c99 so that's what the standard always has to be set to. Older android sdks use an old compiler where this was not yet the default standard, so it has to be set explicitly.

Aidan63 commented 2 months ago

Yes, mbedtls is written in c99 so that's what the standard always has to be set to. Older android sdks use an old compiler where this was not yet the default standard, so it has to be set explicitly.

Thanks, I've added that change to the build.xml in this branch as well.

Simn commented 2 months ago

Please update the branch!

tobil4sk commented 2 months ago

Note, lime has previously had issues with the hxcpp mbedtls version conflicting with its own mbedtls version when an hxcpp project is statically linked. https://github.com/openfl/lime/pull/1767

So I'm not sure if this may be considered a breaking change. For example if this is released with hxcpp 4.3.3, then a new release of lime can support hxcpp 4.3.2 or 4.3.3 but potentially not both (at least not without possible issues).

Aidan63 commented 2 months ago

I guess this could be held back to haxe 5, but I don't see that coming out this year which means hxcpp will be left using an EOL tls library.

Could lime not shadow cpp.NativeSsl and do some version checks to add build xml for either the hxcpp mbedtls or its own version? (I have zero lime / openfl knowledge)

tobil4sk commented 2 months ago

Could lime not shadow cpp.NativeSsl and do some version checks to add build xml for either the hxcpp mbedtls or its own version? (I have zero lime / openfl knowledge)

The problem is cpp side rather than haxe side. Lime has its own mbedtls version which it has internally in its ndll, separate from hxcpp's mbedtls used by the haxe standard library. However, in static builds only one mbedtls can exist, so hxcpp's mbedtls gets overwritten by lime's during linking. This means that all haxe standard library calls end up using lime's mbedtls.

The problem that occurred previously was that lime updated its internal version to mbedtls 3 while hxcpp is still using mbedtls 2. So haxe standard library calls were forced to use mbedtls 3 which resulted in segmentation faults. Lime ended up reverting to mbedtls 2 for the time being. If hxcpp is updated to mbedtls 3 now, current versions of lime will interfere with (and possibly break) any Haxe standard library calls that use mbedtls.

This is a bit of a weird situation though so I'm not sure what's best to do here, and I agree that hxcpp shouldn't use eol software. This can't be solved with conditional compilation either, because the issue happens at link time. I'll try to investigate whether lime can simply use hxcpp's version of mbedtls.

Simn commented 2 months ago

I propose that we release hxcpp 4.3.3 as-is and then merge this PR afterwards. That way we can point to a specific hxcpp version that works with mbedtls2.

tobil4sk commented 2 months ago

I propose that we release hxcpp 4.3.3 as-is and then merge this PR afterwards. That way we can point to a specific hxcpp version that works with mbedtls2.

This would be good. It would be helpful also if this release before mbedtls 3 includes some changes to make it easier for hxcpp's mbedtls (and other static libs) to be used externally. This would be the proper way to allow lime (and other projects) to avoid these types of linking issues, then it will make it less of a concern once the migration to mbedtls 3 occurs.

Also, minor note, but I think following Hugh's convention, the next hxcpp release is whatever the tag number is (e.g. 4.3.45).

tobil4sk commented 1 month ago

Thanks for bearing with all the xml changes. They should allow for lime to work around the mbedtls issue, so it should be possible to avoid the problem caused by hxcpp updating to mbedtls 3.