mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 70749: Introduced optional new scheme for hostname validation.
Date Fri, 21 Jun 2019 01:32:54 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message