obs-outputs: Add mbedTLS support to libRTMP

[compiler-errors]

When compiled with the flag -DWITH_RTMPS=ON, Cmake will look for mbedTLS (specifically, the static libs). This should compile with any modern version of mbedTLS.

Works out-of-box with macOS. Needs mbedtls installed with brew just for building/linking, but should statically link after built.

To build on Linux, mbedTLS needs to be built from scratch with -fPIC and with the dynamic linking flags off so it generates *.a files. Same as above, this is just needed for building. Alternatively, dynamically linking with the mbedTLS *.so files included in any common linux package manager and adding mbedTLS dependency on Linux is also an option.

To build on Windows, mbedtls.lib needs to be built using the visual studio project that comes with the mbedTLS sources. Once that's done, you can just throw the mbedtls.lib file and the include/ directory into the same DepsPath that already consolidates all of the windows dependencies.

Finally, I also changed the Facebook stream URL to point to the RTMPS url instead of the RTMP url.

[jp9000]

The commit messages are a tiny bit different than what we'd usually use. Although you're modifying the librtmp library, the module being modified for the first two is technically obs-outputs. The commit messages are typically 50 character titles (minus prefix), blank line, then the commit description, which is word-wrapped at 72 columns.

So for your commits, I'd probably suggest something more along the lines of:

obs-outputs: Add support for and use mbedTLS for SSL

This diff adds mbedTLS support.  PolarSSL and mbedTLS have grown so
different between 2015-or-so when libRTMP was written and now that it's
no longer feasible to just use the USE_POLARSSL flag that is present in
libRTMP already.  I kept the old PolarSSL flag, but now there's
USE_MBEDTLS instead which uses the library more responsibly due to the
introduction of a bunch of _init and _free functions, etc.

Right now we're statically linking, meaning that the dependency on
mbedTLS on the client machine is not necessary and which alleviates
concerns of shipping OBS with this flag on by default.

obs-outputs: Fix OS-specific mbedTLS cmake rules

CMake rule for mbedTLS should work correctly with the build system on
Linux, MacOS, and Windows.

rtmp-services: Change Facebook stream URL to use RTMPS

You'll notice they're wrapped at 72 characters, and the titles are a bit shorter and more concise.

---

Now, for the cmake/Modules/FindMbedTLS.cmake file, I'd probably suggest this instead, which is normally what we'd use to help with our windows dependencies:

https://gist.github.com/jp9000/ed5b96a52bd7e8bbc515f69bca216c30

This would allow us to easily package mbedtls as part of the dependencies zip on windows. You may have to change MBEDTLS_FOUND to LIBMBEDTLS_FOUND in the plugins/obs-outputs/CMakeLists.txt as well.

---

Last issue is the custom send callback, RTMP::m_customSendFunc. You'll notice that, if it's set, it'll be called in the WriteN() function in rtmp.c (normally set to socket_queue_data() in plugins/obs-outputs/rtmp-stream.c) instead of RTMPSockBuf_Send(), which means that TLS_write() may not be called for that case. Instead, it calls the normal send() function in plugins/obs-outputs/rtmp-windows.c, lines 172 and 176. A suggested fix would probably be to make check those two lines call RTMPSockBuf_Send() instead of send() directly.

[compiler-errors]

@jp9000, I fixed my commit messages and addressed the new Windows networking stack send() issue you mentioned. I've tested on all three OSes, and it seems to work fine.

I adjusted that cmake file you provided in the gist with some OS-specific peculiarities I had found. I've commented them in the FindMbedtls cmake file pretty well, but I also wanted to mention them here:

• Windows: The mbedtls Visual Studio solution generates one *.lib file, instead of the three *.a files found on MacOS and Linux. This means that I had to add a few cases in the cmake file to handle exporting either one or three files.
• Linux and MacOS: I set the _MBED*_LIBRARIES variables to their libmbed*.a equivalent, or else find_library() will prefer the dynamically-linked version which we do not want.
• Linux-only: At the end, I wrap the dependency with -Wl,--whole-archive, or else the linker won't include a statically linked library (libmbed*.a) in the obs-outputs.so file. Commented in the file.

A lot of these issues are due to the fact that (as far as I understand), we want to statically link mbedtls into the obs-outputs plugin.

[compiler-errors]

Note: Also, anyone who has checked out these sources, I git push --force'ed my sources so I could reword the commit stack, so make sure you git fetch --all && git reset --hard origin/master if you want to avoid weird merges due to the diverged master branch.

[notr1ch]

Could this be set to MBEDTLS_SSL_VERIFY_REQUIRED? Disabling certificate verification negates the security of an RTMPS connection. I'm not entirely sure how mbed TLS handles root certs, I suppose we'd need to ship a CA bundle?

[jp9000]

I'm going to try the PR with that param. Hopefully @compiler-errors can make some time verify this.

[jp9000]

I tested the MBEDTLS_SSL_VERIFY_REQUIRED param and it seemed to work -- @compiler-errors may want to also confirm whether this is alright.

[compiler-errors]

I tested VERIFY_REQUIRED and it doesn't seem to work out-of-the-box. Did you add anything to the client init code to load root certs from the OS?

(tested on MacOS, maybe Linux or Windows know where to look by default?)

[jp9000]

Oh, I'm sorry. What I meant is that it appears to stream. I did get a disconnect my first time I tried it, but I'm unsure if it was due to the cert code change.

[compiler-errors]

So I tested on Linux, and it works there. I tested on MacOS, and it works after I manually load the root certs at /etc/ssl/cert.pem. I'm about to commit an OS-specific case to load this on MacOS, and I'd like to see what @notr1ch thinks.

[notr1ch]

We may want to disable low latency mode if using RTMPS. I'm not entirely sure how mbed TLS handles sizing of records, but writing lots of small chunks may result in a high TLS record overhead.

[RytoEX]

@notr1ch I noticed this comment seemed unresolved. Is this still a concern?

[jp9000]

Hey there @compiler-errors -- when you have time could you take a look at the comments on this pull request?

[jp9000]

I think this may be better if the default is ON for this option. Facebook won't function without it if it's set to rtmps anyway.

[compiler-errors]

Edit: Read new update below

A few things:

Even though I've enabled WITH_RTMPS, the continuous-integration platforms are essentially skipping over the option since they haven't been configured to have the modified build environment (i.e. one that provides mbedTLS static libs). This should get fixed so CI would catch RTMPS-specific bugs.

Commit caec9ab seems like a quick fix to a bigger cert problem in MacOS. The general philosophical issue here is that MacOS uses its own keystores to store all of its encryption info. Luckily, when I load the certs included in /etc/ssl/cert.pem, my Facebook RTMPS tests work, but I'm not sure if this will work for all RTMPS-encrypted servers.

I'll test Windows RTMPS functionality with the VERIFY_REQUIRED option enabled tonight, but I wanted to get this message out sooner than later since I've pushed a commit without much explanation.

I'd like to hear what both of y'all think, @jp9000 @notr1ch.

[compiler-errors]

Hey y'all,

Sorry for all the updates. I actually went ahead and wrote a dedicated RTMP_TLS_LoadCerts() whose functionality should be to load default root certificates from the OS. It seems to work fine on Linux and Mac, but I'm for some reason not loading any certificates on Windows.

I've never worked with the windows crypt32.lib library, so maybe one of y'all has more experience than me.

On another note, for some reason the RTMP_TLS_ctx pointer is non-NULL upon program start in Windows, even though I explicitly set it to NULL. It's probably something I missed in the program init flow, but that cases RTMP_TLS_Init (and thus RTMP_TLS_LoadCerts) to not be called sometimes on Windows... Any ideas?

[compiler-errors]

Works on Linux and MacOS.

[kkartaltepe]

CMake logic should be corrected imo. It claims user has requested WITH_RTMPS when user has not. It does not fail when user manually specifies WITH_RTMPS but the required libraries are not found, and instead disables RTMPS support (this should be done when a user does not specify anything).

findMbedTLS appears to attempt to hijack the library names to inject linker flags. This is not supported and should not be used.

There are walls of deprecation warnings when compiling against mbedtls 2.8.0, I'm not sure what version of Ubuntu is being targeting with the upcoming release but if these deprecated functions and include locations can be avoided I recommend you do.

Currently doesn't appear to compile on ubuntu 18.04 when using dynamic mbedTLS library as suggested it should by in OP Ubuntu ships both static and shared and findMbedTLS has no method of linking the shared libs if desired in this case. The compilation error was due to linking the static libs packaged without fPIC.

[kkartaltepe]

Compiles when built against mbedtls 2.12.0 with fPIC enabled.

And connects and streams successfully to an rtmps endpoint.
--edit--
I have no issues linking and streaming on rtmps without the whole-archive hack, why is this supposedly required?

[RytoEX]

I successfully tested this on Windows 10 64-bit with a 64-bit build of OBS Studio (both Debug and RelWithDebInfo). Seemed to work as expected once I got around some confusion over building mbedTLS.

[compiler-errors]

I also confirmed it works without the --whole-archive flag. I was under the impression it was needed when linking a static library into a shared library, but upon further investigation, this isn't necessary given the linker dependency tree (i.e. given that nothing in the main binary needs mbedTLS, just in obs-outputs). This has been removed.

About the CMake logic, @jp9000 suggested that WITH_RTMPS should be on by default. See the comments on plugins/obs-outputs/CMakeLists.txt from 10 days ago. I'll disable WITH_RTMPS by default for those who are missing the dependencies, and make a STATIC_MBEDTLS cmake option so you can build with static or shared libs by choice.

The "fall back and disable crypto if SSL is missing" logic was already present in OBS when it was using OpenSSL before my changes. I have made it explicitly an error so the builder is aware that they're intending to build with RTMPS and cmake unable to find the correct dependencies.

These options (WITH_RTMPS and MBEDTLS_STATIC) will need to be enabled on the official build flow: the intention that I have discussed with @jp9000 is to ship OBS with RTMPS and mbedTLS enabled by default, since otherwise RTMPS-only targets won't be able to connect on officially-packaged versions of the OBS binary. Statically, too, since we don't want to introduce a extra dependency on the client machine either.

[kkartaltepe]

Sorry if there is a bit of misunderstanding, I don't think building with RTMPS should be disabled by default.
It should first check if the user has the libraries, If the user does have the libraries and has not explicitly disabled RTMPS then build with the feature. If the libraries are not found and RTMPS was enabled explicitly on the command line the build should fail. Finally if the libraries are not found, and nothing was explicitly specified the build should complete without enabling RTMPS.

Basically if the user requests with/without RTMPS it should be respected and an error if it cannot be. If the user doesnt request anything build by default but dont fail if its not found.

[compiler-errors]

Okay! In that case, I'll try to amend it with the default/explicit behavior that you described above. Thanks.

[compiler-errors]

Amended WITH_RTMPS option to take a tri-state (AUTO|ON|OFF) value instead of just boolean ON/OFF, as to emulate the functionality that @kkartaltepe suggested.

[compiler-errors]

(forgot to support the AUTO option everywhere, fixed in 06f288b)

[jp9000]

I'm probably going to squash some of these commits, then merge shortly.

Additionally, there's also a bug where if the user was already using Facebook in their settings, it would still use the non-secure URL. I'm going to fix that up myself after.

[jp9000]

Actually before merging to the repository I may have to submit an email to the BIS due to shipping (or more technically "exporting") mbedTLS. May be a bit before that's finished up, just want to make sure I do it correctly.

[notr1ch]

mbedTLS can be exported without a license.

Ref: https://tls.mbed.org/kb/generic/export-control-eccn-number-for-mbedtls

[kkartaltepe]

In incorporating mbedTLS obs becomes it's own encryption tool which falls under export law. Like mbedTLS it will likely fall under the Open Source exemption, but it still needs to be reported. From note 2: https://www.bis.doc.gov/index.php/policy-guidance/encryption/1-encryption-items-not-subject-to-the-ear

…

On Thu, Aug 2, 2018, 09:40 Richard Stanway ***@***.***> wrote: mbedTLS can be exported without a license. Ref: https://tls.mbed.org/kb/generic/export-control-eccn-number-for-mbedtls — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgIU7YKmF7pdDnZrOTdCd3WPSUazoIDks5uMyuJgaJpZM4VIGm5> .

[notr1ch]

I've filed a Self Classification Report just to be sure then. The code can safely be used under the mass market exception.

[jp9000]

Okay, everything looks good, tested it and everything seems to be working smoothly. Going to squash this myself, and fix up a code styling issue or two. Merging shortly, you should see something like "closed from [commit]".

[jp9000]

Additionally, there was a bug where if the user was set to the old server, it would get stuck on that server and would not use RTMPS. I made a fix for that here: be8ddc0

[RytoEX]

@compiler-errors
I wanted to follow-up on this. Is it still at all possible to compile OBS with RTMPS support with libraries other than mbedTLS at this point? There are still ifdefs in librtmp for GnuTLS and OpenSSL, but the OBS CMake files now seem to only support mbedTLS, unless I'm misunderstanding. This question really only pertains to those who want to self-compile on platforms supported by those other libraries, and I wanted to confirm my understanding of this behavior.

[compiler-errors]

Right now there isn't a simple way to build with another SSL library (e.g. GnuTLS).

It would be possible to modify the CMake logic to allow that to happen, but I wanted to keep it simple. Before this PR, the CMake logic also only allowed OpenSSL, so I don't think OBS ever let you choose your SSL library easily.

[RytoEX]

@compiler-errors
Fair point. Thanks for answering. As I said, I mostly wanted to confirm my understanding for the present situation.