> On July 19, 2017, 11:50 a.m., Jan-Philip Gehrcke wrote: > > 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"
re 4: We commonly use past tense for the RR subject and present tense for the description. We also always terminate with a period. Both the summary and the description form the commit message as a whole. Hence it should be "Added support for OpenSSL's ECDHE key exchange." - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60913/#review180916 ----------------------------------------------------------- On July 26, 2017, 9:25 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60913/ > ----------------------------------------------------------- > > (Updated July 26, 2017, 9:25 a.m.) > > > Review request for mesos, Jie Yu, James Peach, and Till Toenshoff. > > > Bugs: MESOS-7792 > https://issues.apache.org/jira/browse/MESOS-7792 > > > Repository: mesos > > > Description > ------- > > This patch adds the configuration necesary so that the Elliptic Curve > Diffie Hellman algorithm can be used for TLS key exchange if the > OpenSSL version used supports it. > > It also adds the ssl flag `LIBPROCESS_SSL_ECDH_CURVES` which allows > for the specification of a specific elliptic curve (or set of curves). > > Users will need to specify the TLS cipher suite that uses ECDH in order > to enable the new key exchange. By default Mesos does not use any ECDH > cipher suites. > > Support for ephemeral ECDH public keys is the default, so that new > public keys are generated for each exchange. > > Note that in order to enable ECDSA ciphers an ECDSA is still necesary > instead of the more traditionl RSA one. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/ssl/flags.hpp > 13fa7a0cc9d6d6d6849976a3ce383263c51504d7 > 3rdparty/libprocess/src/openssl.hpp > 7ded2c74b2f92aacfa0f366bd27d5e0df2b8f25c > 3rdparty/libprocess/src/openssl.cpp > e6f17e4591f573186e1dc9697e1e7b60a841fe4f > 3rdparty/libprocess/src/tests/ssl_tests.cpp > 8a14dcb865dfab34fb4d0d51f42a28a913fb7ace > > > Diff: https://reviews.apache.org/r/60913/diff/7/ > > > 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 > >
