-
Notifications
You must be signed in to change notification settings - Fork 196
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
mbedtls3 migration #1113
base: master
Are you sure you want to change the base?
mbedtls3 migration #1113
Conversation
Co-authored-by: Zeta <53486764+Apprentice-Alchemist@users.noreply.github.com>
Co-authored-by: Zeta <53486764+Apprentice-Alchemist@users.noreply.github.com>
Co-authored-by: Zeta <53486764+Apprentice-Alchemist@users.noreply.github.com>
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.
Annoyingly that cppia stage of the CI compiles absolutely fine on locally on my PC, so more investigation needed there... |
Revert "try compiling mbedtls in its own files tag against a c std version" This reverts commit fe6ec81.
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. 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). |
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. |
Please update the branch! |
Note, lime has previously had issues with the hxcpp mbedtls version conflicting with its own mbedtls version when an hxcpp project is statically linked. openfl/lime#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). |
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) |
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. |
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. |
src/hx/libs/ssl/mbedtls_config.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is useful to have this file containing only the flags that we have set differently from the default values. The default configuration is already included in include/mbedtls/mbedtls_config.h
. Having the entire configuration here again makes it harder to keep track of what we actually need to set ourselves, and it makes it harder to update the config file when we update mbedtls in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see you've replaced MBEDTLS_USER_CONFIG_FILE
with MBEDTLS_CONFIG_FILE
which is why the entire config is required now. I'd still suggest that it's easier to keep track of defines we've changed if we have the default config and then our own small config file which just changes the values that we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I misunderstood the migration docs. It only talked about using MBEDTLS_CONFIG_FILE
so assumed that was the new config method. I've reverted back to MBEDTLS_USER_CONFIG_FILE
.
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). |
src/hx/libs/ssl/Build.xml
Outdated
<compilerflag value="-I${this_dir}"/> | ||
<compilerflag value="-std=c99" unless="MSVC_VER" /> | ||
<compilerflag value="-I${this_dir}"/> | ||
<compilerflag value="-DWIN32_LEAN_AND_MEAN" if="windows"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into this issue when porting neko as well. It may make more sense to put this definition directly in threading_alt.h
, where it is needed:
HaxeFoundation/neko@31e9db6#diff-c5bad8f9ad3067b73cb76e84d84c5fef6f46ad66600fa9f8356ecfc0240158fd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shifting the lean and mean define to threading_alt for hxcpp doesn't do the trick. You'll still get redefinition errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this patch on my machine and hxcpp_ssl was compiling fine: tobil4sk@a51f140. It's not breaking the windows ci either: https://github.com/tobil4sk/hxcpp/actions/runs/9809645213/job/27088061829
The reason I would prefer to have it in threading_alt.h
is that reducing the number of command line flags required to compile hxcpp's mbedtls makes it easier for lime to link against it, which would solve lime's mbedtls issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave it another try and it works now. I can't remember where the error I was getting was coming from since I tried moving the lean and mean define around the place pretty early on in this branch. Probably some compiler cache invalidation issue.
project/thirdparty/mbedtls-flags.xml
Outdated
|
||
<depend name="${HXCPP}/project/thirdparty/mbedtls-flags.xml" dateOnly="true" /> | ||
|
||
<compilerflag value="-I${HXCPP}/project/thirdparty/mbedtls-2.28.2/include" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include path has to be updated here as well now. I think eventually we should switch to using submodules and just have a mbedtls
directory rather than mbedtls-x.x.x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely wouldn't want to see a change to submodules, they are hell on earth and it would be better if the git world forgot they existed. Switching to a sane alternative like subtrees would be good though.
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. |
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.