Add Mbed TLS Support

[Sean-Der]

This is still a WIP. If anyone wants to follow my progress/has feedback just want to make it public

[tytan652]

I think this PR needs to be tested against MbedTLS 2 and 3.

Edit: This thought is not related to OBS, it's just because 2 is still a supported LTS. But supporting only 3 is fine.

[Sean-Der]

@tytan652 What version of mbedtls does OBS use today?

I am hesitant to support mbed tls 2 because it has things like DTLS 1.0. I don't want to encourage/support insecure endpoints https://bugs.chromium.org/p/webrtc/issues/detail?id=10261

[tytan652]

@tytan652 What version of mbedtls does OBS use today?

Both (2 and 3), because Linux distro usually stick with 2 for long-term purpose and some provides 3.

[tytan652]

But you can support only 3 if you want and use GnuTLS/OpenSSL for Linux, we already do that for FFmpeg (one different TLS lib per OS).

Edit: Just specify the minimum version in the find_package in this case (which usually requires a finder).

[Sean-Der]

Thanks @tytan652. Your choice @paullouisageneau it is your code base!

I can add support for both (if you prefer), but personally I prefer less ifdef and lines of code and push people to update dependencies.

[RytoEX]

@tytan652 What version of mbedtls does OBS use today?

I am hesitant to support mbed tls 2 because it has things like DTLS 1.0. I don't want to encourage/support insecure endpoints https://bugs.chromium.org/p/webrtc/issues/detail?id=10261

On Windows, macOS, and in the Flatpak releases, OBS uses Mbed TLS 3 (currently 3.2.1, but will be keeping up with updates when possible). I believe OBS uses Mbed TLS 2 for our Ubuntu PPA builds. I'm not familiar with what each distro uses for their packages, as the ones I've listed above are the only ones for which we provide direct, official support.

As @tytan652 said above:

But you can support only 3 if you want and use GNUTLS for Linux, we already do that for FFmpeg (one different TLS lib per OS).

[paullouisageneau]

@Sean-Der As I understand, there is no runtime setting in MbedTLS to force a minimal TLS version? That sounds like a weird design.

In that case, I think you may support only version 3, as said earlier people can link against GnuTLS (or OpenSSL) system libraries on Linux.

[Sean-Der]

This compiles/all the WebRTC tests now pass!

I need to implement the Websocket side now. Did this with -DNO_WEBSOCKET

[paullouisageneau]

This compiles/all the WebRTC tests now pass!

I need to implement the Websocket side now. Did this with -DNO_WEBSOCKET

Great news, congrats! Could you please add a Github worflow build-mbedtls.yml when you have finished with the TLS transport?

[Sean-Der]

@paullouisageneau done!

I am gonna head to bed. In the morning will fix any test failures that have come up. When everything passes I think we are ready for a review!

[tytan652]

A CMake finder for Mbed TLS is still missing, CMake and Mbed TLS does not provide their own. You need to provide to libdatachannel this finder.

[Sean-Der]

Ok addressed all comments @paullouisageneau

re: tagging a release. We have a little bit of a chicken and egg problem

• I need to get a version into obs-deps so people can test
• I can't fully know if things are going to work across all platforms, so might need to make more libdatachannel changes

I would like to get a release so I can move ahead with OBS, but I might need to come back with other fixes if I hit something. Would you want to do a beta or rc tag? Does that make things easier on your side

[RytoEX]

Following up on this review thread, posting as a standalone comment for visibility:

Following up on this, I noticed that the finder was initially pulled from the linked repository/PR. While the finder included here has since been modified, if an explicitly licensed file is preferred as the basis of the work here (the finder previously linked to did not have an explicit license), you can find MIT-licensed finders here.

• I need to get a version into obs-deps so people can test

On the obs-deps side, our WIP PR for this effort is currently just pulling in a commit from your fork instead of from this repo. We will continue to do that until we no longer need to.

[Sean-Der]

@RytoEX Fixed the Mbed TLS finder. The diff contained no functional changes, but now attributes properly.

[paullouisageneau]

I would like to get a release so I can move ahead with OBS, but I might need to come back with other fixes if I hit something. Would you want to do a beta or rc tag? Does that make things easier on your side

@Sean-Der A beta tag sounds fine to me!

[Sean-Der]

@paullouisageneau done! thank you so much for the fast reviews

[paullouisageneau]

You must not call again on WANT_READ or WANT_WRITE as it would spin when no incoming data is available.

As I understand, you should call again immediately on X_IN_PROGRESS and return on WANT_READ or WANT_WRITE (while scheduling after the timeout for DTLS).

This should be the behavior for each call to mbedtls_ssl_handshake and mbedtls_ssl_read.

[Sean-Der]

@paullouisageneau If I remove those all websocket tests fail with. I also get DTLS failures of the same sort

2023-03-29 21:09:26.016 ERROR [130773] [rtc::mbedtls::check@106] MbedTLS error: SSL - No data of requested type currently available on underlying transport
2023-03-29 21:09:26.016 ERROR [130773] [rtc::impl::TlsTransport::doRecv@458] TLS recv: MbedTLS error: SSL - No data of requested type currently available on underlying transport

[paullouisageneau]

Hum, SSL - No data of requested type currently available on underlying transport is actually MBEDTLS_ERR_SSL_WANT_READ, so it would mean MBEDTLS_ERR_SSL_WANT_READ ends up in mbedtls::check() instead of triggering a return.

[paullouisageneau]

I think the last issue is with mbedtls_ssl_handshake and mbedtls_ssl_read return value handling here, otherwise it looks good to me!

After fixing the issue, I'll merge the PR and create a tag.

[Sean-Der]

Ok fixed! @paullouisageneau fingers crossed I think I got it all right :D

Thanks for all your patience on the PR. Getting WHIP into OBS is going to be so great for people I can't wait.

If someone can broadcast to their friends/co-workers/x$ without setting up a expensive server it is going to be massive for people. Really excited.

[paullouisageneau]

Ok fixed! @paullouisageneau fingers crossed I think I got it all right :D

@Sean-Der Looks good, could you just fix the DTLS handshake and read too before merging?

Thank you for your work and patience! WHIP in OBS is indeed very exciting, but I know Mbed TLS support will also unlock new possibilities for a lot of people.

[Sean-Der]

@paullouisageneau done! I think I got it this time :D

[paullouisageneau]

It looks good, thank you!

[paullouisageneau]

@Sean-Der Tagged as v0.19.0-alpha.1

[pkviet]

Thanks from obs devs. @paullouisageneau