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

migrate to mbedtls 3.x with mbedtls 2.x backward compatibility #9662

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

awakecoding
Copy link
Member

@awakecoding awakecoding commented Dec 15, 2023

This pull request adds support for mbedtls 3.x while retaining mbedtls 2.x compatibility. mbedtls support is still limited to WinPR library builds, as we don't yet have full support for making FreeRDP hybrid openssl/mbedtls builds in one go. For our internal builds, we build WinPR+mbedtls first, then FreeRDP+openssl against WinPR+mbedtls. Fixing the build system for this would be better off done in a separate pull request, and start with mbedtls 3/2 compatibility.

Here is how I've built mbedtls 2 and 3 locally:

git clone git@github.com/Mbed-TLS/mbedtls.git
cd mbedtls

git checkout v3.5.1
mkdir build-mbedtls3 && cd build-mbedtls3
cmake -DENABLE_TESTING=OFF -DENABLE_PROGRAMS=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_INSTALL_PREFIX="/opt/deps/mbedtls3" ..
make
sudo make install
cd ..

git checkout v2.28.6
mkdir build-mbedtls2 && cd build-mbedtls2
cmake -DENABLE_TESTING=OFF -DENABLE_PROGRAMS=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_INSTALL_PREFIX="/opt/deps/mbedtls2" ..
make
sudo make install
cd ..

Here is how I have built WinPR against both versions of mbedtls locally (yes, the CMakeLists.txt in FreeRDP/WinPR can be used as the "root" one for WinPR-only builds):

cd FreeRDP/winpr
mkdir build-mbedtls2 && cd build-mbedtls2
cmake -DCMAKE_PREFIX_PATH="/opt/deps/mbedtls2" -DCMAKE_INSTALL_PREFIX="/opt/deps/winpr-mbedtls2" -DWITH_MBEDTLS=ON -DWITH_OPENSSL=OFF ..
make
sudo make install

cd FreeRDP/winpr
mkdir build-mbedtls3 && cd build-mbedtls3
cmake -DCMAKE_PREFIX_PATH="/opt/deps/mbedtls3" -DCMAKE_INSTALL_PREFIX="/opt/deps/winpr-mbedtls3" -DWITH_MBEDTLS=ON -DWITH_OPENSSL=OFF ..
make
sudo make install

And now, to build FreeRDP against a prebuilt WinPR (this requires a bit of fiddling in cmake/ConfigureFreeRDP.cmake that I'll look into a future pull request for the build system improvements):

cd FreeRDP
mkdir build-mbedtls3 && cd build-mbedtls3
cmake -DCMAKE_PREFIX_PATH="/opt/deps/mbedtls3;/opt/deps/winpr-mbedtls3" -DCMAKE_INSTALL_PREFIX="/opt/deps/winpr-mbedtls3" -DWITH_MBEDTLS=ON -DWITH_OPENSSL=OFF -DFREERDP_UNIFIED_BUILD=OFF -DWITH_SERVER=OFF ..

That's it! Most of the changes were to remove deprecated algorithms, and I also removed a bunch of definition mappings we were likely never going to use (I originally had replicated the entire set from mbedtls). Some struct members became private, requiring the usage of functions to access them. I added backward compatibility wrappers such that it can still build with mbedtls 2.

Since RC4 and MD4 were removed from mbedtls, I set the default to use internal RC4, MD4 for mbedtls, while leaving them on (previous default) for openssl builds. The old havege random number generated was also removed from mbedtls, so I added code for the newer APIs which aren't as straightforward to use, but should do the trick.

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/10970/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/10971/

winpr/include/winpr/custom-crypto.h Outdated Show resolved Hide resolved
winpr/libwinpr/crypto/cipher.c Outdated Show resolved Hide resolved
winpr/libwinpr/crypto/cipher.c Show resolved Hide resolved
winpr/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/10973/

@awakecoding
Copy link
Member Author

@akallabeth changes have been made according to comments, as for the removal of mbedtls id mapping of DES, Blowfish ciphers, they've been removed in mbedtls 3.x, and it's not worth using ifdefs for compatibility. As for camelia ciphers, we never needed them, and they're bound to be removed eventually, so might as well rip the band aid now. I do use the AES ciphers in other projects that use WinPR without FreeRDP though, so we're keeping those.

@akallabeth
Copy link
Member

@awakecoding your changes break the openssl build on the ci.

@awakecoding
Copy link
Member Author

I'll do another pass of changes on Monday to get this pull request clean and ready to go. I'll put the camellia ciphers back, but blowfish and DES will still be removed because they're no longer defined, is that okay? In other words I'll limit the ID removal to what's been removed in mbedtls 3.x

@akallabeth
Copy link
Member

@awakecoding I´d just keep all the defines in the public header, but no need to implement that (no longer existing) ones for mbedtls. (just return a failure where appropriate)
just want to keep the headers and defined symbols consistent for 3.0

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11000/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11001/

@awakecoding
Copy link
Member Author

@akallabeth it should all be good now!

Copy link
Member

@akallabeth akallabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akallabeth akallabeth added this to the stable-3.0-next milestone Dec 19, 2023
@akallabeth akallabeth merged commit 6fbd447 into FreeRDP:master Dec 19, 2023
3 checks passed
@akallabeth
Copy link
Member

@awakecoding great, and merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants