----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70749/#review216017 -----------------------------------------------------------
3rdparty/libprocess/src/openssl.cpp Lines 147 (patched) <https://reviews.apache.org/r/70749/#comment302961> s/algorithm/scheme/? 3rdparty/libprocess/src/openssl.cpp Lines 153 (patched) <https://reviews.apache.org/r/70749/#comment302962> s/algorithm/scheme/? Also, we should tell the user which version he needs at least here, nice touch ;) 3rdparty/libprocess/src/openssl.cpp Lines 565-566 (patched) <https://reviews.apache.org/r/70749/#comment302963> Should we ``` EXIT(EXIT_FAILURE) << "Unknown value for hostname_validation_scheme: " << ssl_flags->hostname_validation_scheme; ``` instead here and below to be perfectly explicit and consistent? 3rdparty/libprocess/src/openssl.cpp Lines 576 (patched) <https://reviews.apache.org/r/70749/#comment302964> s/algorithm/scheme/? 3rdparty/libprocess/src/openssl.cpp Line 755 (original), 788 (patched) <https://reviews.apache.org/r/70749/#comment302965> Maybe ``` // When using the OpenSSL built-in scheme,... ``` 3rdparty/libprocess/src/openssl.cpp Lines 795 (patched) <https://reviews.apache.org/r/70749/#comment302966> s/algorithm/scheme/ 3rdparty/libprocess/src/openssl.cpp Lines 808 (patched) <https://reviews.apache.org/r/70749/#comment302967> I know you just moved it, but where do these 100ms come from and how could we be more explicit about that choice? I would suggest to use a const with some explaining comment how that value was chosen - can we please? :D 3rdparty/libprocess/src/openssl.cpp Lines 984-985 (patched) <https://reviews.apache.org/r/70749/#comment302968> This sounds like a great idea worth spending 1 more cycle on -- can we create and reference a ticket that explains this jazz as nicely as we do here in the code? My thought is that being open about this idea in JIRA, we would provide more chances of getting community support for it. 3rdparty/libprocess/src/openssl.cpp Lines 989 (patched) <https://reviews.apache.org/r/70749/#comment302969> Shall we explain why this is the `right` way - aka best practices? 3rdparty/libprocess/src/openssl.cpp Lines 1016 (patched) <https://reviews.apache.org/r/70749/#comment302970> s/ip/IP/ 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 444-448 (patched) <https://reviews.apache.org/r/70749/#comment302999> Great comment - thanks! 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 569 (patched) <https://reviews.apache.org/r/70749/#comment303000> Now that we log the `peer_hostname`, I wonder if it made sense to also log the `peer_ip` like this - what do you think? If you agree, then we should even try to render a single log-line out of those two - it will be noisy enough on level 2. 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 1129 (patched) <https://reviews.apache.org/r/70749/#comment303002> I feel `peername` comes in a bit surprising here if one does not know about `getpeername()`. Maybe s/peername/IP address/? 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 1136 (patched) <https://reviews.apache.org/r/70749/#comment303001> As mentioned in some other place of this chain, let's add a neat JIRA on this plan. - Till Toenshoff On June 20, 2019, 5:48 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70749/ > ----------------------------------------------------------- > > (Updated June 20, 2019, 5:48 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff. > > > Bugs: MESOS-9809 > https://issues.apache.org/jira/browse/MESOS-9809 > > > Repository: mesos > > > Description > ------- > > This commit introduces a new libprocess SSL flag > `hostname_validation_scheme`, which can be used to select > between the previous hostname validation behaviour and a new > option to use standardized OpenSSL algorithms to handle > hostname validation as part of the handshake. > > As a nice side-effect, the new scheme gets rid of reverse DNS > lookups during TLS connection establishment, which used to be > a common source of hard-to-debug unresponsiveness in Mesos > components. > > See `docs/ssl.md` in the follow-up commit for details of and > differences between the schemes. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/ssl/flags.hpp > f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a > 3rdparty/libprocess/src/openssl.hpp > 17bec246e516261f8d772f1647c17f092fae82d1 > 3rdparty/libprocess/src/openssl.cpp > e7dbd67913fa8e7fbbf60dee428e7e38895f86ce > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp > 6ef5a86566af3439cfe0b06ab3576076623f7be0 > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp > 29a1bf71c1df9d80370455a6269ecea0ec4193b0 > > > Diff: https://reviews.apache.org/r/70749/diff/4/ > > > Testing > ------- > > Todo! > > > Thanks, > > Benno Evers > >
