You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/06/19 14:28:11 UTC
Re: Review Request 70748: Changed semantics of some libprocess TLS
flags.
-----------------------------------------------------------
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
Re: Review Request 70748: Changed semantics of TLS certificate
verification flags.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70748/
-----------------------------------------------------------
(Updated July 2, 2019, 5:28 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Jan-Philip Gehrcke, Joseph Wu, and Till Toenshoff.
Changes
-------
Rebased on master; moved a message to prevent illogical warnings in logs.
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 addtion, 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 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
Diff: https://reviews.apache.org/r/70748/diff/9/
Changes: https://reviews.apache.org/r/70748/diff/8-9/
Testing
-------
See end of this chain.
Thanks,
Benno Evers
Re: Review Request 70748: Changed semantics of TLS certificate
verification flags.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70748/
-----------------------------------------------------------
(Updated June 26, 2019, 10:26 a.m.)
Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, and Till Toenshoff.
Changes
-------
Adjusted info messages to new semantics.
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 addtion, 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/8/
Changes: https://reviews.apache.org/r/70748/diff/7-8/
Testing (updated)
-------
See end of this chain.
Thanks,
Benno Evers
Re: Review Request 70748: Changed semantics of libprocess TLS flags.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70748/
-----------------------------------------------------------
(Updated June 21, 2019, 3:01 p.m.)
Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, and Till Toenshoff.
Summary (updated)
-----------------
Changed semantics of 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 addtion, 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/5/
Changes: https://reviews.apache.org/r/70748/diff/4-5/
Testing
-------
So far, mostly manual testing.
Thanks,
Benno Evers
Re: Review Request 70748: Changed semantics of some libprocess TLS
flags.
Posted by Benno Evers <be...@mesosphere.com>.
> On June 21, 2019, 1:06 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 96 (original), 96 (patched)
> > <https://reviews.apache.org/r/70748/diff/4/?file=2151330#file2151330line96>
> >
> > 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.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70748/#comment302996>
`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)
<https://reviews.apache.org/r/70748/#comment302994>
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)
<https://reviews.apache.org/r/70748/#comment302997>
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)
<https://reviews.apache.org/r/70748/#comment302956>
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)
<https://reviews.apache.org/r/70748/#comment302957>
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
>
>