mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.
Date Thu, 20 Jul 2017 01:52:37 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60913/#review180925
-----------------------------------------------------------



Thanks for this Alexander - this is a great improvement to our TLS.

For getting a better idea how this is done properly, we need to add more information - for
the reviewers as well as the maintainers - that is the biggest takeaway I am hoping to bring
accross here :).


3rdparty/libprocess/src/openssl.cpp
Lines 121 (patched)
<https://reviews.apache.org/r/60913/#comment256415>

    ```
    The curves are in preference order, i.e. the library will try to use the first curve before
the second and so on.
    ```
    
    Not sure if the second part of that sentense improves the first one. I would suggest to
remove that second part entirely.



3rdparty/libprocess/src/openssl.cpp
Lines 122 (patched)
<https://reviews.apache.org/r/60913/#comment256416>

    ```
    Check the OpenSSL documentation for the full list of supported curves.
    ```
    Not sure if this isn't a bit too obvious - again, I would suggest to remove that one entirely.



3rdparty/libprocess/src/openssl.cpp
Lines 126 (patched)
<https://reviews.apache.org/r/60913/#comment256417>

    ```
    NOTE: Old versions of OpenSSL support only one curve, check the documentation of your
OpenSSL.
    ```
    
    FIXME - RESEARCH - FIXME



3rdparty/libprocess/src/openssl.cpp
Lines 296 (patched)
<https://reviews.apache.org/r/60913/#comment256424>

    Can you please reference some project / document that describes that we are following
best practices here? 
    
    We need to be sure that this is the cleanest and most compatible way we can find. I believe
this code originally came from NGINX. We should reference the original and give credit.
    
    Also have you tested this with some older OpenSSL implementations as well as very recent
ones?



3rdparty/libprocess/src/openssl.cpp
Lines 297 (patched)
<https://reviews.apache.org/r/60913/#comment256425>

    Let's make this a `Try<Nothing>` instead so we can properly return errors without
having to exit the process in this setup function.



3rdparty/libprocess/src/openssl.cpp
Lines 299 (patched)
<https://reviews.apache.org/r/60913/#comment256277>

    How about moving this out and guarding the actual `initialize_ecdh_curve` invocation instead?
    This might help making the actual implementation a bit more readable in my opinion.



3rdparty/libprocess/src/openssl.cpp
Lines 304 (patched)
<https://reviews.apache.org/r/60913/#comment256418>

    If used, our trailing comments on such conditionaly included blocks is slightly different;
    
    ```
    #ifdef FOO
    [...]
    #endif // FOO
    ```
    or
    ```
    #if defined(FOO) || defined(BAR) 
    [...]
    #endif // FOO || BAR
    ```



3rdparty/libprocess/src/openssl.cpp
Lines 306 (patched)
<https://reviews.apache.org/r/60913/#comment256279>

    We need to explain why it is fine to continue even if we miss that define.



3rdparty/libprocess/src/openssl.cpp
Lines 317-320 (patched)
<https://reviews.apache.org/r/60913/#comment256426>

    Let's return an `Error` instead - here and everywhere else this setup helper currently
aborts the process.
    
    and within the eror messaage please
    
    s/ecdh/ECDH/



3rdparty/libprocess/src/openssl.cpp
Lines 325 (patched)
<https://reviews.apache.org/r/60913/#comment256428>

    You can drop the namespace for this - s/std:://



3rdparty/libprocess/src/openssl.cpp
Lines 336 (patched)
<https://reviews.apache.org/r/60913/#comment256429>

    Would such error influence `ERR_get_error()` and hence have more details avaiable via
`error_string()`?



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 531 (patched)
<https://reviews.apache.org/r/60913/#comment256265>

    Please add a leading comment on what we are testing here.


- Till Toenshoff


On July 18, 2017, 12:13 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60913/
> -----------------------------------------------------------
> 
> (Updated July 18, 2017, 12:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
>     https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Support for Elliptic Curve Diffie Hellman algorithm requires extra
> configuration parameters which weren't part of Mesos.
> 
> This patch enables the extra configuration to Mesos in order to
> support ECDH algorithm, it also adds the ssl flag
> `LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
> a specific elliptic curve.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 13fa7a0cc9d6d6d6849976a3ce383263c51504d7

>   3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 8a14dcb865dfab34fb4d0d51f42a28a913fb7ace

> 
> 
> Diff: https://reviews.apache.org/r/60913/diff/3/
> 
> 
> Testing
> -------
> 
> ```shell
> make check
> ```
> 
> Launched Mesos with only ECDHE handshake ciphers enabled
> 
> ```shell
> LIBPROCESS_SSL_ENABLED=1 \
> LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
> LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
> LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
\
> ./bin/mesos-master.sh \
>     --work_dir=/tmp/mesos/master \
>     --log_dir=/tmp/mesos/master/log
> ```
> 
> Then in another shell:
> 
> ```shell
> http -v --verify=no https://${MESOS_MASTER_IP}:5050/state
> 
> # Launches a browser.
> open https://${MESOS_MASTER_IP}:5050/state
> 
> # List the set of supported ciphers.
> # Expected output:
> # >  Starting Nmap 7.50 ( https://nmap.org ) at 2017-07-18 11:41 CEST
> # >  Nmap scan report for ${MESOS_MASTER_HOSTNAME} (${MESOS_MASTER_IP})
> # >  Host is up (0.13s latency).
> # >  rDNS record for ${MESOS_MASTER_IP}: ${MESOS_MASTER_HOSTNAME}
> # >  
> # >  PORT     STATE SERVICE
> # >  5050/tcp open  mmcc
> # >  | ssl-enum-ciphers:
> # >  |   TLSv1.2:
> # >  |     ciphers:
> # >  |       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
> # >  |       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
> # >  |       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
> # >  |       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
> # >  |       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
> # >  |       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
> # >  |     compressors:
> # >  |       NULL
> # >  |     cipher preference: server
> # >  |_  least strength: A
> # >  
> # >  Nmap done: 1 IP address (1 host up) scanned in 1.87 seconds
> wget https://svn.nmap.org/nmap/scripts/ssl-enum-ciphers.nse
> nmap --script ssl-enum-ciphers.nse -p 5050 ${MESOS_MASTER_IP}
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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