Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

tls_socket: add set_verify_mode & set_verify_callback #40

Closed binarytrails closed 5 years ago

binarytrails commented 5 years ago

Fixes #39. These functions serves to make the SSL verification with Asio: http://think-async.com/Asio/asio-1.12.2/doc/asio/reference/ssl__rfc2818_verification.html

eao197 commented 5 years ago

@ngrodzitski What do you think about this addition?

ngrodzitski commented 5 years ago

@binarytrails Can you, please, clarify the use case. Are those functions meant to be called from option setter? If so, can you please add an example or describe a scenario for example that will work with self-signed certificates?

ngrodzitski commented 5 years ago

@binarytrails, @eao197 Some clarification on the purpose of tls_socket_t class. It serves as a movable wrapper for asio_ns::ssl::stream< asio_ns::ip::tcp::socket > which is not movable (see https://github.com/chriskohlhoff/asio/issues/124). And the methods it contains now are just for conforming template connection class code which was written for casual socket class primarily. Any method currently exposed in tls_socket_t is used by restinio code (mostly connection_t class).

If my understanding is correct and proposed extra functions are considered to be called from socket option setter, then it is a different case. For example, one would want to use yet another function from asio_ns::ssl::stream class. Adding them one by one is not a practical solution. It would be better to give access to underlying asio_ns::ssl::stream instance itself, so the necessary functions can be called directly without the necessity of being explicitly defined in restinio utility wrapper.

binarytrails commented 5 years ago

@ngrodzitski Absolutely.

We're switching the current OpenDHT RESTbed proxy to RESTinio. However, we are keeping the dependencies very minimal and RESTinio is only a server.

Since your framework code is very clear in its Asio implementation and it can be reused, I built a client with a standard Request class using Asio and RESTinio. The OpenDHT http class has a Connection class which builds a SSL connection. By the means of your tls_socket serving as an adapter for ssl::stream of the tcp::socket, I'm using these two PR methods to establish a secure connection with the handshake as per the standard ssl__rfc2818_verification:

    socket_->set_verify_mode(verify_mode);
    socket_->set_verify_callback(asio::ssl::rfc2818_verification(hostname));
    socket_->async_handshake(asio::ssl::stream<asio::ip::tcp::socket>::client, cb);

I need to use the set_verify_callback method on the ssl::stream which is private in the tls_socket class. Therefore, I simply added these two functions to allow me to use your class to ensure the validity of a certificate for a specific hostname before the handshake.

Cheers, Seva

ngrodzitski commented 5 years ago

@binarytrails Ok, so a function of the following form would do the job?

        socket_t &
        tls_socket() // Or a better name...
        {
            return *m_socket;
        }

Usage:

    socket_->tls_socket().set_verify_mode(verify_mode);
    socket_->tls_socket().set_verify_callback(asio::ssl::rfc2818_verification(hostname));
    socket_->tls_socket().async_handshake(asio::ssl::stream<asio::ip::tcp::socket>::client, cb);
    socket_->tls_socket().whatever_evailable_function(/*...*/)
eao197 commented 5 years ago

I agree with @ngrodzitski that access to underlying Asio's socket is a more appropriate solution than the addition of methods that aren't required for the main RESTinio's functionality.

binarytrails commented 5 years ago

@ngrodzitski @eao197

Yes, I totally agree, I just didn't want to break your private members design.

Do you want me to change it at this PR or you're on it?

eao197 commented 5 years ago

I can make that change tomorrow morning.

binarytrails commented 5 years ago

@eao197 Ok, perfect! Feel free to close this PR after;

Thank you for the prompt reply. Much appreciated.

binarytrails commented 5 years ago

Solved at https://github.com/Stiffstream/restinio/commit/97ce6e36bb1fa74c18e435dc83eb4c18d790dc05