redboltz / mqtt_cpp

Boost Software License 1.0
440 stars 107 forks source link

Support for gnutls #662

Open shantanu1singh opened 4 years ago

shantanu1singh commented 4 years ago

I was able to leverage a fork that I'll try to get merged of this repository (with minimal changes), to support the use of gnutls instead of OpenSSL with the mqtt_cpp library.

Would the owners be accepting of a pull-request that adds support for gnutls using this other repository 'as a reference in docs' or a git submodule?

The changes I've had to make have primarily been the following across files in the mqtt_cpp repo:

1) Include gnutls headers instead of OpenSSL

#if defined(MQTT_USE_TLS)
#if !defined(MQTT_USE_GNU_TLS)
    #include <boost/asio/ssl.hpp>
#else
    #include <boost/asio/gnutls.hpp>
    #include <gnutls/gnutls.h>
#endif // !defined(MQTT_USE_GNU_TLS)
#endif // defined(MQTT_USE_TLS)

2) Change namespace definitions for the ssl library:

namespace as = boost::asio;

#if defined(MQTT_USE_TLS)
#if !defined(MQTT_USE_GNU_TLS)
    namespace ssl = as::ssl;
#else
    namespace ssl = as::gnutls;
#endif // !defined(MQTT_USE_GNU_TLS)
#endif // defined(MQTT_USE_TLS)
jonesmz commented 4 years ago

I think your best path to success for getting support for this merged is going to be to isolate the logic for handling which SSL library to use into a single header file.

Basically just take the stuff you specified in your initial comment here, and put that into ssl_implementation.h, or something similar.

But I don't see why the changes you're proposing would be rejected. This seems like a very useful thing to have.

Further, it also helps with regards to using wolfssl, which is useful for embedded contexts like OpenWRT.

redboltz commented 4 years ago

@shantanu1singh, I'm not sure why you want to use gnutls.

Do you send a pull request for Boost.Asio to support gnutls ? If you did it, please let me know the link of the pull request. If Boost.Asio support gnutls, maybe mqtt_cpp automatically support it via boost. Perhaps small change is required.

This is another point that the license of gnutls is LGPL. It is more constrained licence that Boost license. I don't want to contain LGPL code as the submodule of mqtt_cpp. I guess that it is related topic for #636. The license of WolfTLS is GPLv2 or commacial license.

I'm not sure how Boost.Asio treat the license issue well.

Anyway, I want to keep that mqtt_cpp simply use Boost style.

shantanu1singh commented 4 years ago

@jonesmz With respect to moving the logic to a separate header, I can do that.

@redboltz I'm using mqtt_cpp in a project and our customer is hesitant to use the OpenSSL ssleay license

Boost.Asio doesn't support gnutls right now. This repository provides a GnuTls wrapper for Boost.Asio and it uses the Boost license (Not LGPL)

With the proposed change, there won't be any explicit gnutls related function calls in the mqtt_cpp library. It'll only consume the headers of the wrapper mentioned above. I should have a pull-request ready sometime tomorrow for you to see if it's acceptable.

If you don't like the idea of using a git submodule, could we instead just add instructions in the ReadMe or in the wiki to tell potential users that if they wish to use GnuTLS with mqtt_cpp, they will need to install the headers present in the wrapper above on their device?

redboltz commented 4 years ago

@shantanu1singh , I understand your situation. And I've read https://github.com/paullouisageneau/boost-asio-gnutls carefully. There is no header file name conflict with Boost.Asio, it is good.

I accept

  1. Include gnutls headers instead of OpenSSL

Please don't use negative condition with else clause. See https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules

https://github.com/paullouisageneau/boost-asio-gnutls

  1. Change namespace definitions for the ssl library: It this required? I think that the existing code always use as::ssl not ssl. Am I missing something ?

submodule vs outside

I think that outside file is better for this situation. If user wants to use gnutls, user need to prepare gnutls as the outside module. Similarly, https://github.com/paullouisageneau/boost-asio-gnutls too. Just install it to the system include path or add compiler option for adding include path.

shantanu1singh commented 4 years ago

@redboltz

Please don't use negative condition with else clause. See https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules

Will do

Change namespace definitions for the ssl library: It this required? I think that the existing code always use as::ssl not ssl. Am I missing something ?

Yes, mqtt_cpp currently uses as::ssl, but the equivalent namespace in boost-asio-gnutls is as::gnutls. That's why I was thinking of creating a new namespace alias ssl which refers to boost::asio::ssl for OpenSSL and boost::asio::gnutls when boost-asio-gnutls is used. The side-effect of that though is that I'll have to change all occurrences of as::ssl in mqtt_cpp to ssl. Did you have a better solution in mind?

I think that outside file is better for this situation. If user wants to use gnutls, user need to prepare gnutls as the outside module. Similarly, https://github.com/paullouisageneau/boost-asio-gnutls too. Just install it to the system include path or add compiler option for adding include path.

That works. I can add instructions to the ReadMe in mqtt_cpp or the wiki. Which one would you prefer?

redboltz commented 4 years ago

@redboltz

Please don't use negative condition with else clause. See https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules

Will do

Change namespace definitions for the ssl library: It this required? I think that the existing code always use as::ssl not ssl. Am I missing something ?

Yes, mqtt_cpp currently uses as::ssl, but the equivalent namespace in boost-asio-gnutls is as::gnutls. That's why I was thinking of creating a new namespace alias ssl which refers to boost::asio::ssl for OpenSSL and boost::asio::gnutls when boost-asio-gnutls is used. The side-effect of that though is that I'll have to change all occurrences of as::ssl in mqtt_cpp to ssl. Did you have a better solution in mind?

Thank you for elaborating. I understand. Replacing existing as::ssl is good. I think that newly introduced namespace alias should be tls. I guess that boost asio sub namespace ssl is by historical reason.

I think that outside file is better for this situation. If user wants to use gnutls, user need to prepare gnutls as the outside module. Similarly, https://github.com/paullouisageneau/boost-asio-gnutls too. Just install it to the system include path or add compiler option for adding include path.

That works. I can add instructions to the ReadMe in mqtt_cpp or the wiki. Which one would you prefer?

I think that https://github.com/redboltz/mqtt_cpp/wiki/Config is appropriate place.

In addition, you can add something at TLS support table on README.md.

ineffective commented 3 years ago

@redboltz I'm using mqtt_cpp in a project and our customer is hesitant to use the OpenSSL ssleay license

Currently OpenSSL 3.0.0 and higher is under Apache license.