----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60913/#review180916 -----------------------------------------------------------
Hey! This looks great. I have a few remarks. 1) The test in this patch seems to prove that the patch introduces support for the _ephemeral_ elliptic curve Diffie-Hellman key exchange, usually abbreviated with ECDHE. Does this patch also add ECDH (non-ephemeral key exchange, where an ever-static ECDH public key is used) support? If it does not then I think this is good news :-) I'd appreciate a comment on that in the patch description. If this can be controlled via external configuration through the cipher suite string/list, then this is an appropriate answer. 2) Even ECDHE can be used in two modes. Related to the first point we should stress in the patch description and also via a code comment that this patch explicitly disables an optimization called "ephemeral-static ECDH" via the SSL_OP_SINGLE_ECDH_USE option. I found https://forum.nginx.org/read.php?29,241610,249058 insightful, but also https://eprint.iacr.org/2011/633.pdf from which I'd like to quote: "2. Use of ephemeral ECDH-based cipher suites (e.g., ECDHE-ECDSA and ECDHE-RSA) in combination with the OpenSSL ephemeral-static ECDH optimisation. In such cipher suites, and according to the TLS specification, a fresh ECDH public key should be generated for each key exchange. However OpenSSL allows one-time generation of said key when the TLS server is initialised, sharing it across an arbitrary number of key exchanges thereafter." 3) This patch contains a test that proves that RSA-signed key exchange works (i.e. ECDHE_RSA type cipher suites). I assume that this patch also allows for ECDSA-signed key exchange (i.e. ECDHE_ECDSA type cipher suites), but it just requires a corresponding private key type. I'd appreciate to know if you agree with that or not :-). 4) I think we should adjust the commit message saying "Adds support for OpenSSL's ECDHE key exchange" - Jan-Philip Gehrcke 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 > >
