----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71665/#review219009 -----------------------------------------------------------
3rdparty/libprocess/src/ssl/openssl_socket.hpp Lines 60 (patched) <https://reviews.apache.org/r/71665/#comment307013> Nit: s/should/must/ ? 3rdparty/libprocess/src/ssl/openssl_socket.hpp Lines 71 (patched) <https://reviews.apache.org/r/71665/#comment307014> Nit: put arguments on different lines for consistency. 3rdparty/libprocess/src/ssl/openssl_socket.cpp Line 48 (original), 52 (patched) <https://reviews.apache.org/r/71665/#comment307018> Could you move these rename changes into the patch where the comments were originally added? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Line 285 (original), 295 (patched) <https://reviews.apache.org/r/71665/#comment307041> Nit: use `s` or `_s` consistently here and above. 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 329 (patched) <https://reviews.apache.org/r/71665/#comment307045> Could you add a comment here noting that the destructor is responsible for freeing this? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 433-434 (patched) <https://reviews.apache.org/r/71665/#comment307022> Nit: line wrapping should be updated. 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 436 (patched) <https://reviews.apache.org/r/71665/#comment307019> s/do we not/we do not/ 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 439 (patched) <https://reviews.apache.org/r/71665/#comment307020> Why not run these on the per-socket UPID also? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 481 (patched) <https://reviews.apache.org/r/71665/#comment307040> s/openssl::Mode::SERVER/Mode::SERVER/ 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 493-495 (patched) <https://reviews.apache.org/r/71665/#comment307046> Do you think we should add a timeout here to handle the case of a socket which doesn't complete the handshake? Maybe in a follow-up patch? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 495 (patched) <https://reviews.apache.org/r/71665/#comment307021> Is it correct that from this point on, we don't need to worry about calling `SSL_free()` because ownership of the context has been passed to the socket? If so, could you explain this in a comment? Also a comment in the header where `set_ssl_and_do_handshake` is declared? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 545 (patched) <https://reviews.apache.org/r/71665/#comment307047> Nit: s/socket/sockets/ 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 548-556 (patched) <https://reviews.apache.org/r/71665/#comment307049> Could you make the newlines/indentation consistent across these two callbacks? 3rdparty/libprocess/src/ssl/openssl_socket.cpp Lines 549 (patched) <https://reviews.apache.org/r/71665/#comment307048> Since you create a UPID for the socket later, why not defer to that context here as well? - Greg Mann On Dec. 10, 2019, 11:53 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71665/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2019, 11:53 p.m.) > > > Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till > Toenshoff. > > > Bugs: MESOS-10010 > https://issues.apache.org/jira/browse/MESOS-10010 > > > Repository: mesos > > > Description > ------- > > This fills in some of the SSL socket implementation, > in particular the constructor, destructor, connect(), > and accept() methods. > > Much of the setup and verification is taken verbatim from the > libevent socket implementation. > > A change to the poll socket was necessary to prevent the SSL > socket from holding a self-reference indefinitely. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > 48860f8646d388685f0a60ad2a2f613b1f4be61a > 3rdparty/libprocess/src/posix/poll_socket.cpp > ecc2bd492c4edd2f6ab0aae52d50bb3954881893 > 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION > 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION > 3rdparty/libprocess/src/windows/poll_socket.cpp > e2a84694ac554b4c23242fd93d93800c0334a943 > > > Diff: https://reviews.apache.org/r/71665/diff/6/ > > > Testing > ------- > > cmake --build . --target libprocess-tests > > Successfully connected to Google :D > With something like this: > ``` > set_environment_variables({ > {"LIBPROCESS_SSL_ENABLED", "true"}, > {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, > {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()} > }); > > Try<Socket> client = Socket::create(SocketImpl::Kind::SSL); > ASSERT_SOME(client); > > AWAIT_ASSERT_READY(client->connect( > network::inet::Address(net::IP::parse("216.58.194.206").get(), 443), > openssl::create_tls_client_config(None()))); > ``` > > > Thanks, > > Joseph Wu > >
