Re: Review Request 70748: Changed semantics of some libprocess TLS flags.

2019-06-21 Thread Benno Evers 


> On June 21, 2019, 1:06 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 96 (original), 96 (patched)
> > 
> >
> > Additional to the updates above, we also need to call this out in the 
> > CHANGELOG. Can you please include that in this chain?

I added the `CHANGELOG` changes in a new review 
https://reviews.apache.org/r/70921/


- Benno


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


On June 19, 2019, 2:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
> 
> (Updated June 19, 2019, 2:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Bugs: MESOS-9810
> https://issues.apache.org/jira/browse/MESOS-9810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit slightly updates the semants of the
> `LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
> environment variables. The former now only applies to connections
> in client mode and the latter now only applies to connections in
> server mode.
> 
> In particular, in TLS server mode we now *only* verify client
> certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
> regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.
> 
> In addition, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
> has been set to `true`, enforce that the server actually presents a
> certificate that can be verified. Note that this is expected to be
> not a behavioural change in practice, since the TLS specification
> already states that a server MUST always send a certificate unless an
> anonymous cipher is used, and most TLS ciphersuites are configured to
> exclude anonymous ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 
> 
> 
> Diff: https://reviews.apache.org/r/70748/diff/4/
> 
> 
> Testing
> ---
> 
> So far, mostly manual testing.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70748: Changed semantics of some libprocess TLS flags.

2019-06-20 Thread Till Toenshoff via Review Board 

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


Fix it, then Ship it!




Great - thanks so much!


3rdparty/libprocess/src/openssl.cpp
Line 90 (original), 90 (patched)


`verify_cert` effectively becomes a TLS client mode specific setting - we 
should explain that here. In my mind, in TLS server mode, `verify_cert` stricly 
follows `require_cert` in its state.



3rdparty/libprocess/src/openssl.cpp
Line 95 (original), 95 (patched)


This is a TLS server mode only setting by now - we need to call that out I 
feel. In TLS client mode, we have `require_cert` enabled implicitly.



3rdparty/libprocess/src/openssl.cpp
Line 96 (original), 96 (patched)


Additional to the updates above, we also need to call this out in the 
CHANGELOG. Can you please include that in this chain?



3rdparty/libprocess/src/openssl.cpp
Lines 743-745 (patched)


This is so important, I would like to add more meat to it;

Let's clarify that the OpenSSL handshake itself will do this, before we 
even reach our additional verification. Also, the use of `remote` confuses me - 
do local servers have an exception here? 

Maybe something like this?
```
  // NOTE: Even without this check, the OpenSSL handshake will
  // not complete when connecting to servers that do not present
  // a certificate, unless an anonymous cipher is used.
```



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 296-297 (patched)


Nice, thank you!


- Till Toenshoff


On June 19, 2019, 2:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
> 
> (Updated June 19, 2019, 2:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Bugs: MESOS-9810
> https://issues.apache.org/jira/browse/MESOS-9810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit slightly updates the semants of the
> `LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
> environment variables. The former now only applies to connections
> in client mode and the latter now only applies to connections in
> server mode.
> 
> In particular, in TLS server mode we now *only* verify client
> certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
> regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.
> 
> In addition, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
> has been set to `true`, enforce that the server actually presents a
> certificate that can be verified. Note that this is expected to be
> not a behavioural change in practice, since the TLS specification
> already states that a server MUST always send a certificate unless an
> anonymous cipher is used, and most TLS ciphersuites are configured to
> exclude anonymous ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 
> 
> 
> Diff: https://reviews.apache.org/r/70748/diff/4/
> 
> 
> Testing
> ---
> 
> So far, mostly manual testing.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70748: Changed semantics of some libprocess TLS flags.

2019-06-19 Thread Benno Evers 

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

(Updated June 19, 2019, 2:28 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Changes
---

Updated commit description.


Summary (updated)
-

Changed semantics of some libprocess TLS flags.


Bugs: MESOS-9810
https://issues.apache.org/jira/browse/MESOS-9810


Repository: mesos


Description (updated)
---

This commit slightly updates the semants of the
`LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
environment variables. The former now only applies to connections
in client mode and the latter now only applies to connections in
server mode.

In particular, in TLS server mode we now *only* verify client
certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.

In addition, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
has been set to `true`, enforce that the server actually presents a
certificate that can be verified. Note that this is expected to be
not a behavioural change in practice, since the TLS specification
already states that a server MUST always send a certificate unless an
anonymous cipher is used, and most TLS ciphersuites are configured to
exclude anonymous ciphers.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70748/diff/4/

Changes: https://reviews.apache.org/r/70748/diff/3-4/


Testing (updated)
---

So far, mostly manual testing.


Thanks,

Benno Evers