----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70991/#review216342 -----------------------------------------------------------
Overall approach looks good, thanks for the update! Some comments below, but I feel comfortable at this point with other reviewers looking things over. 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 26-28 (patched) <https://reviews.apache.org/r/70991/#comment303515> Stale comment? Connect requires the tls config, right? 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 62-63 (patched) <https://reviews.apache.org/r/70991/#comment303516> Some guidance on what these are and how one should use them would be helpful, probably on the variables here (or if on the types above, perhaps a comment here pointing the reader to the above explanations) 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 82 (patched) <https://reviews.apache.org/r/70991/#comment303517> `LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME=libprocess` seems a little strange, can we call the "old" way `legacy` to signal that it's deprecated and we want to remove it? 3rdparty/libprocess/src/http.cpp Line 1452 (original), 1454-1465 (patched) <https://reviews.apache.org/r/70991/#comment303514> How about: ``` Future<Nothing> connected = [&]() { switch (scheme) { case Scheme::HTTP: return socket->connect(address); #ifdef USE_SSL_SOCKET case Scheme::HTTPS: return socket->connect( address, network::openssl::create_tls_client_config(peer_hostname)); #endif } UNREACHABLE(); // If needed to silence compiler. }(); ``` 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 518 (patched) <https://reviews.apache.org/r/70991/#comment303513> Ditto here w.r.t. EXIT 3rdparty/libprocess/src/posix/poll_socket.cpp Lines 167 (patched) <https://reviews.apache.org/r/70991/#comment303512> EXIT will not produce a stack trace, can you use LOG(FATAL) here so that we can see who made the incorrect call? Generally, we don't use EXIT for programming errors for this reason 3rdparty/libprocess/src/process.cpp Lines 1453 (patched) <https://reviews.apache.org/r/70991/#comment303511> s/std::// 3rdparty/libprocess/src/tests/http_tests.cpp Line 263 (original), 264-276 (patched) <https://reviews.apache.org/r/70991/#comment303518> Ditto here, can use the lambda approach? 3rdparty/libprocess/src/tests/socket_tests.cpp Lines 68 (patched) <https://reviews.apache.org/r/70991/#comment303509> no need for the break anymore? 3rdparty/libprocess/src/tests/socket_tests.cpp Lines 73 (patched) <https://reviews.apache.org/r/70991/#comment303519> Any comment on why we don't pass a hostname here or if we should? Do these tests even exercise the SSL socket? Will the branch be executed? 3rdparty/libprocess/src/tests/socket_tests.cpp Lines 75 (patched) <https://reviews.apache.org/r/70991/#comment303510> ditto here, no need for the break? 3rdparty/libprocess/src/tests/ssl_client.cpp Line 147 (original), 147-157 (patched) <https://reviews.apache.org/r/70991/#comment303520> Ditto here for assignment from result of a lambda? 3rdparty/libprocess/src/tests/ssl_client.cpp Lines 155 (patched) <https://reviews.apache.org/r/70991/#comment303521> Ditto here on a comment about why we don't pass hostname here 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 559 (patched) <https://reviews.apache.org/r/70991/#comment303523> For all these spots passing None(), should we explain? ``` const Future<Nothing> connect = client.connect( server_address.get(), // E.g. Don't care about hostname certificate validation? openssl::create_tls_client_config(None())); ``` 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 1008-1022 (patched) <https://reviews.apache.org/r/70991/#comment303524> Should we also verify the configure/verify error cases surfacing correctly as errors? - Benjamin Mahler On July 2, 2019, 5:50 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70991/ > ----------------------------------------------------------- > > (Updated July 2, 2019, 5:50 p.m.) > > > Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff. > > > Bugs: MESOS-9878 > https://issues.apache.org/jira/browse/MESOS-9878 > > > Repository: mesos > > > Description > ------- > > Reworked the API of `Socket::connect()` according to the following > boundary conditions requested by libprocess maintainers: > > * It shall be possible to use custom connection options when > connecting with a SSL socket. > * When libprocess is compiled without SSL support, neither the > declaration of the TLS configuration object nor the `connnect()` > overload that accepts the TLS configuration should be available. > * Passing just the servername is not an acceptable short-hand for > using the default TLS configuration together with that servername. > * When the incorrect overload is selected (i.e. passing TLS config > to a poll socket or omitting TLS configuration for a TLS socket), > the program should abort. > > This commit changes the API of `Socket::connect()` according to the > requirements above. In particular: > > * A new class `openssl::TLSClientConfig` is introduced when libprocess > is compiled with ssl support. > * A new overload > `Socket::connect(const Address&, const TLSClientConfig&)` is > introduced when libprocess is compiled with ssl support. > * All call sites are adjusted to check the socket kind before calling > `connect()`. > > > Diffs > ----- > > 3rdparty/libprocess/include/Makefile.am > 1ddcc2d5a30f7bf3914138e497a9b228b515cd29 > 3rdparty/libprocess/include/process/socket.hpp > 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 > 3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION > 3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d > 3rdparty/libprocess/src/openssl.hpp > 17bec246e516261f8d772f1647c17f092fae82d1 > 3rdparty/libprocess/src/openssl.cpp > 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 > 3rdparty/libprocess/src/poll_socket.hpp > 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp > 6ef5a86566af3439cfe0b06ab3576076623f7be0 > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp > 7e2229a9ed815727500bd457356e5531607fa6cf > 3rdparty/libprocess/src/posix/poll_socket.cpp > 74acb6942682a9d9626df81b303eba0a1c24ecf7 > 3rdparty/libprocess/src/process.cpp > 799666f03d6a78708aa9336c2dd04bc9b5023aa0 > 3rdparty/libprocess/src/tests/http_tests.cpp > 4d372943a2d417d24d06444ec2e72909fb348017 > 3rdparty/libprocess/src/tests/socket_tests.cpp > b09ae23a551c6587656b2d5f6f58c5267e8e0088 > 3rdparty/libprocess/src/tests/ssl_client.cpp > de87b3b89c84d17f2ebba1f09e9ec682f139aace > 3rdparty/libprocess/src/tests/ssl_tests.cpp > 5d360221937e68da185754f0633fa41a217c7107 > > > Diff: https://reviews.apache.org/r/70991/diff/1/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
