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

Using TLSv1 since boost1_41 supports TLS v1.0 #218

Merged
merged 2 commits into from
Feb 22, 2017
Merged

Conversation

jai1
Copy link
Contributor

@jai1 jai1 commented Feb 16, 2017

Motivation

TLS code in current master was incompatible with C++98 and boost 1.41

Modifications

Removed C++11 features
Used TLSv1.0 from boost 1.41

Result

the code can be compiled on an env with C++98 and boost1.41

@jai1
Copy link
Contributor Author

jai1 commented Feb 16, 2017

@yush1ga - can you please have a look at these changes

@@ -32,7 +32,7 @@ typedef boost::shared_ptr<boost::asio::ssl::stream<boost::asio::ip::tcp::socket&
typedef boost::shared_ptr<boost::asio::ip::tcp::resolver> TcpResolverPtr;
typedef boost::shared_ptr<boost::asio::deadline_timer> DeadlineTimerPtr;
class ExecutorService : private boost::noncopyable {

friend class ClientConnection;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rdhabalia - We need it inorder to access ExecutorService.io_service_

@@ -114,13 +114,13 @@ isTlsAllowInsecureConnection_(false) {
if (clientConfiguration.isUseTls()) {
using namespace boost::filesystem;

boost::asio::ssl::context ctx(boost::asio::ssl::context::tlsv12_client);
boost::asio::ssl::context ctx(executor_->io_service_, boost::asio::ssl::context::tlsv1_client);

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to figure out the latest supported version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, at least, we could add an #ifdef block to select the TLS version depending on the boost version.

Copy link
Contributor

@yush1ga yush1ga Feb 16, 2017

Choose a reason for hiding this comment

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

boost::asio::ssl::context ctx(boost::asio::ssl::context::tlsv1_client);

Doesn't this work on boost 1.41 ?

context(
    boost::asio::io_service & ,
    method m);

This constructor seems to be deprecated recent version.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a macro available to identify boost version, we could use that to pick different version.

@merlimat merlimat added the c++ label Feb 17, 2017
@merlimat merlimat added this to the 1.17 milestone Feb 17, 2017
boost::asio::ssl::context ctx(boost::asio::ssl::context::tlsv12_client);

#if BOOST_VERSION >= 105400
boost::asio::ssl::context ctx(executor_->io_service_, boost::asio::ssl::context::tlsv12_client);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still confused on why we need to pass the io_service here, in particular the one from the executor which is kind of unrelated to the connection itself.

As for the macro, I'd prefer to have it defined in the top section of the file. Something like

#if BOOST_VERSION >= 105400
   #define TLS_CLIENT = boost::asio::ssl::context::tlsv12_client
#else
   #define TLS_CLIENT = boost::asio::ssl::context::tlsv1_client
#endif

Copy link
Contributor Author

@jai1 jai1 Feb 21, 2017

Choose a reason for hiding this comment

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

Boost 1.41 has only one constructor for creating the context and this constructor requires io_service_

Current version of boost have rewritten the entire ssl code and don't require the io_service as a part of the constructor.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@jai1 jai1 merged commit 1691778 into apache:master Feb 22, 2017
@merlimat
Copy link
Contributor

@jai1 Please use "Squash and merge" when merging a PR, unless there is specific need to keep multiple commits separated

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.

5 participants