Add support for GnuTLS

[shantanu1singh]

Add support for GnuTLS

The feature is controlled through the variable MQTT_USE_GNU_TLS. MQTT_USE_TLS also needs to be enabled for this feature to work.

If MQTT_USE_GNU_TLS is disabled and MQTT_USE_TLS is enabled, mqtt_cpp will use OpenSSL instead of GnuTLS

[shantanu1singh]

Related to issue #662

[jonesmz]

I have two suggestions

1. Can you re-write your git-history to be a bit more concise? E.g. "Working changes" seems like it could have been merged into a different commit.

2. What do you think about using MQTT_TLS_IMPL=GnuTLS instead of MQTT_USE_GNU_TLS=ON ?
   MQTT_TLS_IMPL would default to OpenSSL, but then other implementations could be provided without needing to add more and more build-options.

[jonesmz]

This isn't right

You're overwriting the flags that are already selected for the "1" case.

If you want to add the GNUTLS implementation to the unit tests, you'll need to add more items to the list "matrix.pattern", and not reuse them.

[jonesmz]

My recommendation is to shrink the text matrix a bit.

Right now we have "MQTT_TEST_1" through MQTT_TEST_7". Most likely these can be merged down into a smaller list, perhaps 1 though 5 or something. That'll keep the combinatorial explosion slightly under control.

[shantanu1singh]

I can reduce the test list but I wasn't sure which ones are okay to remove. If you and @redboltz both agree to keep it 1-5 only, I can easily update it

[jonesmz]

I think you might have misunderstood.

My recommendation was to re-arrange the MQTT_TEST_1 and so on binaries so that the code for each of them is preserved, and all the tests continue working, but there are fewer build varients.

[shantanu1singh]

Sorry, not sure I'm fully following your recommendation.

Did you mean rearranging the patterns such that they are ordered in a way where all patterns that have MQTT_TEST_1
enabled are one after another?

[jonesmz]

Specifically I meant changing text/CMakeLists.txt to have the different MQTT_TEST_# entries have different cpp files included in them.

[jonesmz]

I have two suggestions

1. Can you re-write your git-history to be a bit more concise? E.g. "Working changes" seems like it could have been merged into a different commit.

I think this order of doing things makes sense, to me anyway,

1. Add the tls_implementation.h file and have only the macros needed for openssl.
2. Introduce the "tls" namespace, convert all locations where as::ssl is used.
3. Any places in the code that need to behave differently based on the ssl implementation, should have free-standing functions that take the appropriate arguments added, and be converted to use those free functions. Implement only the OpenSSL logic at this point.
4. Adjust tls_implementation.h to support also the GnuTLS implementation. Adjust the various ifdefs, macros, namespaces, and free functions. This commit would have no changes happen to the main mqtt_cpp code, only the tls_implementation.h file
5. Add unit tests and cmake infastructure.

[codecov]

Codecov Report

Merging #663 into master will increase coverage by 0.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
+ Coverage   82.13%   82.68%   +0.55%     
==========================================
  Files          45       46       +1     
  Lines        7012     7076      +64     
==========================================
+ Hits         5759     5851      +92     
+ Misses       1253     1225      -28     

[redboltz]

@shantanu1singh , I'm not sure the current situation.
Could you rebase the PR from the new master and cleanup commit history ?

When you add new concurrent builds, you need to avoid queuing. Because it makes longer CI times. It depends on github actions. So please check the number of maximum concurrent tasks. I think that it is independently limited by osx, linux, and windows. So far, windows build is not used on github actions. It is implemented azure pipelines. The reason I use the azure pipelines, it is for faster CI times. I want to avoid queuing tasks.

For gnutls, I think that the following tests are good enough. They are focused on connecting sequence.

https://github.com/redboltz/mqtt_cpp/blob/master/test/connect.cpp
https://github.com/redboltz/mqtt_cpp/blob/master/test/underlying_timeout.cpp

I think that other part of the code, there is no difference between gnutls and openssl.

The tests are grouped in MQTT_TEST_1.

You need to add only one line that contains MQTT_TEST_1 and gnutls setting for each os. Other tests and example should be OFF.

[shantanu1singh]

@redboltz, I'm a bit preoccupied with work. I'll get back to this PR sometime next week or the one after.
I'll update the PR with rebased commits too. Currently running into a problem with some test failures so I need to debug those.

You need to add only one line that contains MQTT_TEST_1 and gnutls setting for each os. Other tests and example should be OFF.

Sounds good, I'll use just those tests then for gnutls

[redboltz]

@redboltz, I'm a bit preoccupied with work. I'll get back to this PR sometime next week or the one after.

No problem. Take your time.

As #663 (comment) commented, you can do that as follows:

See "Compiler options:" textbox on the left side of the code.

Compiler option	Result
no tls option	https://wandbox.org/permlink/z5FhFWckXyRMpgmE
-DMQTT_USE_TLS	https://wandbox.org/permlink/ejf6fahORc0zPKVN
-DMQTT_USE_TLS=MQTT_TLS_OPENSSL	https://wandbox.org/permlink/exmogJXC8Zti43SA
-DMQTT_USE_TLS=MQTT_TLS_GNUTLS	https://wandbox.org/permlink/gjxyCDS6jQ3fwwnu

[shantanu1singh]

@redboltz thanks for the tip! I'll use that

I had one question - With respect to including the headers in boost-asio-gnutls, the PR in it's current state assumes that the headers in this repo have been placed under the boost/asio directory on the machine.

I was wondering if it's a good idea to require consumers to mix default boost/asio headers with the headers from this repository. What do you think about introducing a new CMake Variable that allows consumers to specify the path to the directory boost-asio-gnu-tls on the consumer's machine? That way we can use the variable and add it to the include_directories path of the mqtt_cpp CMake if GNU TLS is enabled.

Something like:

-DWITH_BOOST_ASIO_GNU_TLS=/usr/local/include/boost-asio-gnutls

[shantanu1singh]

@redboltz thanks for the tip! I'll use that

I had one question - With respect to including the headers in boost-asio-gnutls, the PR in it's current state assumes that the headers in this repo have been placed under the boost/asio directory on the machine.

I was wondering if it's a good idea to require consumers to mix default boost/asio headers with the headers from this repository. What do you think about introducing a new CMake Variable that allows consumers to specify the path to the directory boost-asio-gnu-tls on the consumer's machine? That way we can use the variable and add it to the include_directories path of the mqtt_cpp CMake if GNU TLS is enabled.

Something like:

-DWITH_BOOST_ASIO_GNU_TLS=/usr/local/include/boost-asio-gnutls

Hey @redboltz , what do you think about the above?

[redboltz]

@redboltz thanks for the tip! I'll use that
I had one question - With respect to including the headers in boost-asio-gnutls, the PR in it's current state assumes that the headers in this repo have been placed under the boost/asio directory on the machine.
I was wondering if it's a good idea to require consumers to mix default boost/asio headers with the headers from this repository. What do you think about introducing a new CMake Variable that allows consumers to specify the path to the directory boost-asio-gnu-tls on the consumer's machine? That way we can use the variable and add it to the include_directories path of the mqtt_cpp CMake if GNU TLS is enabled.
Something like:
-DWITH_BOOST_ASIO_GNU_TLS=/usr/local/include/boost-asio-gnutls

Hey @redboltz , what do you think about the above?

I think that it is a good idea. I think that MQTT_BOOST_ASIO_GNUTLS_INCLUDE is good name.
Prefix MQTT is to avoid conflict. GNUTLS respects the repository name. *_INCLUDE is typical way to set include path on cmake.

[jonesmz]

There's no need to delete this. Github doesn't reuse virtual machines.

That being said, why bother with the copy? I'd rather you just clone the repository directly to the destination. https://stackoverflow.com/questions/600079/how-do-i-clone-a-subdirectory-only-of-a-git-repository

And you should make sure to specify --depth=1, to avoid large downloads.

Alternatively, if somehow doing a direct clone isn't possible, then I'd rather it be a move operation. You can just "mv" the entire "include" folder directly to /usr/local/include/boost-asio-gnutls.

Lastly, why does this even need to be moved to /usr/local ? Can't you just specify the full path ? -DMQTT_BOOST_ASIO_GNUTLS_INCLUDE=$(realpath boost-asio-gnutls/include)

[shantanu1singh]

sure, I can do that too.

[jonesmz]

No, don't add this. Builds should not happen inside the source tree.

[shantanu1singh]

The repository's ReadMe suggests doing just that. I've seen several other projects follow the same format.

What's so wrong with adding the build/ directory to this file?

[jonesmz]

At the very least, if it's going to be added to the repository, it needs to happen in it's own pull request.

There are a variety of reasons why builds should not be done inside the source directory, but one specific one that I'll share to help you research the topic, is that cmake natively understands multiple configurations. If you put the build inside the source directory, you pre-clude the ability to have multiple build configurations at the same time.

[shantanu1singh]

I can remove it. It's just that it was making my life easier as I've always built libraries this way.

[jonesmz]

Why namespace the include directory like this?

If you need the ability to insert additional include directories, just name it "MQTT_ADDITIONAL_INCLUDE" or something, and take list as the argument.

Are you sure CMake doesn't already have a specific feature for this built in?

[shantanu1singh]

Couldn't find a native Cmake way of doing that.

As per this stack overflow post, the other option we have is to let consumers set CXX flags which doesn't seem too appealing.

I can change it to be MQTT_ADDITIONAL_INCLUDES and accept a list if that works

[jonesmz]

Personally I don't see the benefit of making something that clearly applies in a generic way named in a feature specific way.

I think it'll cause more confusion to people to have it named MQTT_BOOST_ASIO_GNUTLS_INCLUDE.

@redboltz can you offer some guidance?

[redboltz]

I got confused. I don't understand what you want to do, so far.
Do you simply want to add include path of GNUTLS ?
If it does, -DCMAKE_CXX_FLAGS+="-I path_to_gnutls".

[shantanu1singh]

I ran into a problem with websocket based clients i.e., MQTT_USE_TLS combined with MQTT_USE_WS.

The header boost/beast/websocket/gnutls/ssl.hpp included in include/mqtt/ws_endpoint.hpp refers to boost/asio/ssl/stream.hpp. That's based on OpenSSL.

I had to update two headers of boost/beast/websocket to use the 'boost-asio-gnutls' headers instead and the updated files can be found here
Is it acceptable to use the headers referred to in the linked repository in the mqtt_cpp library?

Essentially, we'll include the following headers in ws_endpoint.hpp (or rather tls_implementation.hpp), based on the appropriate SSL library:

OpenSSL

#include <boost/beast/websocket/ssl.hpp>

GnuTLS

#include <boost/beast/websocket/gnutls/ssl.hpp>

@redboltz @jonesmz any suggestions are welcome.

[jonesmz]

Any chance of convincing boost beast to fix their header, instead of working around the problem in mqtt_cpp ?

[shantanu1singh]

@jonesmz Not sure if they'd be accepting of that change so easily. They'd make the argument of updating boost/asio to add support for GnuTLS natively instead of leveraging a different repository 'boost-asio-gnutls' entirely.

[jonesmz]

Ok.

Well, ultimately @redboltz makes the call.

My advice is that before this change is accepted into mqtt_cpp, the appropriate PRs to get GNUTls integrated into boost asio and boost beast be created, so that there's a roadmap in place for when the integration patches inside of mqtt_cpp can be dropped.

[redboltz]

@jonesmz Not sure if they'd be accepting of that change so easily. They'd make the argument of updating boost/asio to add support for GnuTLS natively instead of leveraging a different repository 'boost-asio-gnutls' entirely.

I think that it is better that boost asio and boost beast support gnutls directly. Because TLS support is very generic topic. If boost asio and beast users such as mqtt_cpp implements gnutls support individually, the situation would be confused (code duplication etc).

Do someone or you already proposed to boost to support gnutls ?

[redboltz]

I have some question about TLS and WesSocket combination. Now I'm checking on #673 .
I guess that beast doesn't need to care about TLS because it is hidden underlying layer from the beast.
Anyway please wait a moment.

Also I consider #663 (comment) comment. I'm looking for a good way.

[redboltz]

I guess that beast doesn't need to care about TLS because it is hidden underlying layer from the beast.

I was wrong. Due to very complected web socket and TLS tear down sequence, beast needs to care about TLS layer directly.
Graceful shutdown is a headache on Websocket on TLS on TCP.

[redboltz]

@shantanu1singh , could you try #673 ?

As I commented at #663 (comment), GnuTLS support is implemented by boost asio and boost beast, not mqtt_cpp.
So I implemented minimal support code for GnuTLS by mqtt_cpp.

You can test and use as follows.

cmake -DCMAKE_CXX_FLAGS="-DMQTT_TLS_INCLUDE=<boost/asio/gnutls.hpp> -DMQTT_TLS_WS_INCLUDE=<boost/beast/websocket/gnutls.hpp> -DMQTT_TLS_NS=boost::asio::gnutls" other_cmake_options... target_path

So you don't need to add tests. Just use #673 by your responsibility.

[shantanu1singh]

@jonesmz Not sure if they'd be accepting of that change so easily. They'd make the argument of updating boost/asio to add support for GnuTLS natively instead of leveraging a different repository 'boost-asio-gnutls' entirely.

I think that it is better that boost asio and boost beast support gnutls directly. Because TLS support is very generic topic. If boost asio and beast users such as mqtt_cpp implements gnutls support individually, the situation would be confused (code duplication etc).

Do someone or you already proposed to boost to support gnutls ?

@redboltz No, I personally have not nor do I know of someone who did. The reason I started looking into it was just that we are using the mqtt_cpp library in our project and our customer doesn't wish to use OpenSSL. Getting support for GnuTLS incorporated into boost will likely be a bigger task I think. I might open a ticket on their repository to see how they feel about it

[shantanu1singh]

I guess that beast doesn't need to care about TLS because it is hidden underlying layer from the beast.

I was wrong. Due to very complected web socket and TLS tear down sequence, beast needs to care about TLS layer directly.
Graceful shutdown is a headache on Websocket on TLS on TCP.

@redboltz Could you possibly link me to some references that explain what you meant? Is there some teardown process that I would need to handle with GnuTLS if I use it with mqtt_cpp using #673 ?

[shantanu1singh]

@shantanu1singh , could you try #673 ?

As I commented at #663 (comment), GnuTLS support is implemented by boost asio and boost beast, not mqtt_cpp.
So I implemented minimal support code for GnuTLS by mqtt_cpp.

You can test and use as follows.

cmake -DCMAKE_CXX_FLAGS="-DMQTT_TLS_INCLUDE=<boost/asio/gnutls.hpp> -DMQTT_TLS_WS_INCLUDE=<boost/beast/websocket/gnutls.hpp> -DMQTT_TLS_NS=boost::asio::gnutls" other_cmake_options... target_path

So you don't need to add tests. Just use #673 by your responsibility.

@redboltz Thank you for sharing that. I can try using it. Although, would you be merging that PR with master?

[redboltz]

@shantanu1singh , could you try #673 ?
As I commented at #663 (comment), GnuTLS support is implemented by boost asio and boost beast, not mqtt_cpp.
So I implemented minimal support code for GnuTLS by mqtt_cpp.
You can test and use as follows.

cmake -DCMAKE_CXX_FLAGS="-DMQTT_TLS_INCLUDE=<boost/asio/gnutls.hpp> -DMQTT_TLS_WS_INCLUDE=<boost/beast/websocket/gnutls.hpp> -DMQTT_TLS_NS=boost::asio::gnutls" other_cmake_options... target_path

So you don't need to add tests. Just use #673 by your responsibility.

@redboltz Thank you for sharing that. I can try using it. Although, would you be merging that PR with master?

Yes, if it works well on your environment.
#673 is minimal support for alternative TLS library. The library needs to provide compatible interfaces as boost/asio/ssl.hpp and boost/beast/websocket/ssl.hpp.

[redboltz]

I guess that beast doesn't need to care about TLS because it is hidden underlying layer from the beast.

I was wrong. Due to very complected web socket and TLS tear down sequence, beast needs to care about TLS layer directly.
Graceful shutdown is a headache on Websocket on TLS on TCP.

@redboltz Could you possibly link me to some references that explain what you meant? Is there some teardown process that I would need to handle with GnuTLS if I use it with mqtt_cpp using #673 ?

For example, boost beast directly use ssl feature as follows:

https://github.com/boostorg/beast/blob/develop/include/boost/beast/websocket/ssl.hpp
https://github.com/boostorg/beast/blob/develop/include/boost/beast/websocket/impl/ssl.hpp

You might think why MQTT_TLS_WS_NS is not required. I guess that there is no explicit public interfaces for boost beast websocket (TLS) users. The special treatment requires only teardown process. I guess that teardown process is called from (some class') destructor automatically (implicitly).

Unfortunately, I don't know what kind of special treatment is required.

[redboltz]

@shantanu1singh, does #673 work well ?

[shantanu1singh]

@redboltz I haven't had the chance to test it yet. Will likely get to it today or tomorrow. I'll keep you posted here.

[shantanu1singh]

@redboltz I tried out #673 but it failed with an error. I've pointed out what needs to be changed in your PR. If you can help update that, I can give it another test. Thanks!

[shantanu1singh]

This is the error just as a reference:

/home/shantanu/repos/mqtt_cpp_1/include/mqtt/endpoint.hpp: In member function ‘bool mqtt::endpoint<Mutex, LockGuard, PacketIdBytes>::handle_close_or_error(mqtt::error_code)’:
/home/shantanu/repos/mqtt_cpp_1/include/mqtt/endpoint.hpp:4567:17: error: there are no arguments to ‘ERR_GET_REASON’ that depend on a template parameter, so a declaration of ‘ERR_GET_REASON’ must be available [-fpermissive]
             || (ERR_GET_REASON(ec.value()) == boost::asio::ssl::error::stream_truncated)
                 ^~~~~~~~~~~~~~
/home/shantanu/repos/mqtt_cpp_1/include/mqtt/endpoint.hpp:4567:17: note: (if you use ‘-fpermissive’, G++ will accept your code, but allowing the use of an undeclared name is deprecated)
/home/shantanu/repos/mqtt_cpp_1/include/mqtt/endpoint.hpp:4567:60: error: ‘boost::asio::ssl’ has not been declared
             || (ERR_GET_REASON(ec.value()) == boost::asio::ssl::error::stream_truncated)

[redboltz]

Could you post a new PR to fix #673 ? The target branch is add_alt_tls_support.