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/07/02 17:50:56 UTC
Review Request 70991: Updated `Socket::connect()` API according to
maintainer feedback.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70991/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
Repository: mesos
Description
-------
Reworked the API of `Socket::connect()` according to the following
boundary conditions requested by libprocess maintainers:
* It shall be possible to use custom connection options when
connecting with a SSL socket.
* When libprocess is compiled without SSL support, neither the
declaration of the TLS configuration object nor the `connnect()`
overload that accepts the TLS configuration should be available.
* Passing just the servername is not an acceptable short-hand for
using the default TLS configuration together with that servername.
* When the incorrect overload is selected (i.e. passing TLS config
to a poll socket or omitting TLS configuration for a TLS socket),
the program should abort.
This commit changes the API of `Socket::connect()` according to the
requirements above. In particular:
* A new class `openssl::TLSClientConfig` is introduced when libprocess
is compiled with ssl support.
* A new overload
`Socket::connect(const Address&, const TLSClientConfig&)` is
introduced when libprocess is compiled with ssl support.
* All call sites are adjusted to check the socket kind before calling
`connect()`.
Diffs
-----
3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
Diff: https://reviews.apache.org/r/70991/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 70991: Updated `Socket::connect()` API according to
maintainer feedback.
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/70991/#review216383
-----------------------------------------------------------
3rdparty/libprocess/src/poll_socket.hpp
Lines 16-18 (patched)
<https://reviews.apache.org/r/70991/#comment303604>
No need for #ifdef.
3rdparty/libprocess/src/poll_socket.hpp
Lines 16-18 (patched)
<https://reviews.apache.org/r/70991/#comment303605>
No need for #ifdef.
- Till Toenshoff
On July 2, 2019, 5:50 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> -----------------------------------------------------------
>
> (Updated July 2, 2019, 5:50 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9878
> https://issues.apache.org/jira/browse/MESOS-9878
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reworked the API of `Socket::connect()` according to the following
> boundary conditions requested by libprocess maintainers:
>
> * It shall be possible to use custom connection options when
> connecting with a SSL socket.
> * When libprocess is compiled without SSL support, neither the
> declaration of the TLS configuration object nor the `connnect()`
> overload that accepts the TLS configuration should be available.
> * Passing just the servername is not an acceptable short-hand for
> using the default TLS configuration together with that servername.
> * When the incorrect overload is selected (i.e. passing TLS config
> to a poll socket or omitting TLS configuration for a TLS socket),
> the program should abort.
>
> This commit changes the API of `Socket::connect()` according to the
> requirements above. In particular:
>
> * A new class `openssl::TLSClientConfig` is introduced when libprocess
> is compiled with ssl support.
> * A new overload
> `Socket::connect(const Address&, const TLSClientConfig&)` is
> introduced when libprocess is compiled with ssl support.
> * All call sites are adjusted to check the socket kind before calling
> `connect()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
> 3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
> 3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
> 3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
> 3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
> 3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
> 3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
> 3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
> 3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
> 3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
>
>
> Diff: https://reviews.apache.org/r/70991/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70991: Added ability to pass custom SSL context to
`Socket::connect()`.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70991/
-----------------------------------------------------------
(Updated July 5, 2019, 9:03 a.m.)
Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
Changes
-------
Add some wording changes.
Bugs: MESOS-9878
https://issues.apache.org/jira/browse/MESOS-9878
Repository: mesos
Description
-------
Users of libprocess can now pass a custom SSL context when
connecting a generic socket via the `Socket::connect()`
function.
Additionally the API of `Socket::connect()` was also reworked
according to the following boundary conditions requested by
libprocess maintainers:
* When libprocess is compiled without SSL support, neither the
declaration of the TLS configuration object nor the `connnect()`
overload that accepts the TLS configuration should be available.
* Passing just the servername is not an acceptable short-hand for
using the default TLS configuration together with that servername.
* When the incorrect overload is selected (i.e. passing TLS config
to a poll socket or omitting TLS configuration for a TLS socket),
the program should abort.
This following changes are introduced according to the requirements
above:
* A new class `openssl::TLSClientConfig` is introduced when libprocess
is compiled with ssl support.
* A new overload
`Socket::connect(const Address&, const TLSClientConfig&)` is
introduced when libprocess is compiled with ssl support.
* All call sites are adjusted to check the socket kind before calling
`connect()`.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
Diff: https://reviews.apache.org/r/70991/diff/3/
Changes: https://reviews.apache.org/r/70991/diff/2-3/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 70991: Added ability to pass custom SSL context to
`Socket::connect()`.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
> On July 4, 2019, 11:53 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 806 (original), 807 (patched)
> > <https://reviews.apache.org/r/70991/diff/2/?file=2153776#file2153776line807>
> >
> > s/libprocess/legacy/
Actually, please make sure you dont end up with
```
// i.e. the lagacy 'legacy' scheme
```
:D
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70991/#review216396
-----------------------------------------------------------
On July 4, 2019, 7 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> -----------------------------------------------------------
>
> (Updated July 4, 2019, 7 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9878
> https://issues.apache.org/jira/browse/MESOS-9878
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Users of libprocess can now pass a custom SSL context when
> connecting a generic socket via the `Socket::connect()`
> function.
>
> Additionally the API of `Socket::connect()` was also reworked
> according to the following boundary conditions requested by
> libprocess maintainers:
>
> * When libprocess is compiled without SSL support, neither the
> declaration of the TLS configuration object nor the `connnect()`
> overload that accepts the TLS configuration should be available.
> * Passing just the servername is not an acceptable short-hand for
> using the default TLS configuration together with that servername.
> * When the incorrect overload is selected (i.e. passing TLS config
> to a poll socket or omitting TLS configuration for a TLS socket),
> the program should abort.
>
> This following changes are introduced according to the requirements
> above:
>
> * A new class `openssl::TLSClientConfig` is introduced when libprocess
> is compiled with ssl support.
> * A new overload
> `Socket::connect(const Address&, const TLSClientConfig&)` is
> introduced when libprocess is compiled with ssl support.
> * All call sites are adjusted to check the socket kind before calling
> `connect()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
> 3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
> 3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
> 3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
> 3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
> 3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
> 3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
> 3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
> 3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
> 3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
>
>
> Diff: https://reviews.apache.org/r/70991/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70991: Added ability to pass custom SSL context to
`Socket::connect()`.
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/70991/#review216396
-----------------------------------------------------------
Fix it, then Ship it!
Awesome work Benno - thanks!
Can't wait to see these great additions landing.
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 59 (patched)
<https://reviews.apache.org/r/70991/#comment303611>
```
(See also the comments on the typedefs above.)
```
Looking at the callback type comments is somewhat implicit, i feel - we can do without that part.
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 65-69 (patched)
<https://reviews.apache.org/r/70991/#comment303613>
I would suggest to split this up into smaller digestable sentences - native speaker please?!
Otherwise maybe like this?
```
// Returns `TLSClientConfig` object configured with provided `servername` as
// well as the global libprocess SSL context. The callbacks for `verify` and
// `configure_socket` get setup following the SSL behaviour configured via
// the `LIBPROCESS_SSL_*` environment variables.
```
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 80 (patched)
<https://reviews.apache.org/r/70991/#comment303614>
s/libprocess/legacy/
3rdparty/libprocess/src/openssl.cpp
Line 806 (original), 807 (patched)
<https://reviews.apache.org/r/70991/#comment303615>
s/libprocess/legacy/
- Till Toenshoff
On July 4, 2019, 7 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> -----------------------------------------------------------
>
> (Updated July 4, 2019, 7 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9878
> https://issues.apache.org/jira/browse/MESOS-9878
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Users of libprocess can now pass a custom SSL context when
> connecting a generic socket via the `Socket::connect()`
> function.
>
> Additionally the API of `Socket::connect()` was also reworked
> according to the following boundary conditions requested by
> libprocess maintainers:
>
> * When libprocess is compiled without SSL support, neither the
> declaration of the TLS configuration object nor the `connnect()`
> overload that accepts the TLS configuration should be available.
> * Passing just the servername is not an acceptable short-hand for
> using the default TLS configuration together with that servername.
> * When the incorrect overload is selected (i.e. passing TLS config
> to a poll socket or omitting TLS configuration for a TLS socket),
> the program should abort.
>
> This following changes are introduced according to the requirements
> above:
>
> * A new class `openssl::TLSClientConfig` is introduced when libprocess
> is compiled with ssl support.
> * A new overload
> `Socket::connect(const Address&, const TLSClientConfig&)` is
> introduced when libprocess is compiled with ssl support.
> * All call sites are adjusted to check the socket kind before calling
> `connect()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
> 3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
> 3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
> 3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
> 3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
> 3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
> 3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
> 3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
> 3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
> 3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
>
>
> Diff: https://reviews.apache.org/r/70991/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70991: Added ability to pass custom SSL context to
`Socket::connect()`.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70991/
-----------------------------------------------------------
(Updated July 4, 2019, 7 p.m.)
Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
Changes
-------
Addressed review comments.
Summary (updated)
-----------------
Added ability to pass custom SSL context to `Socket::connect()`.
Bugs: MESOS-9878
https://issues.apache.org/jira/browse/MESOS-9878
Repository: mesos
Description (updated)
-------
Users of libprocess can now pass a custom SSL context when
connecting a generic socket via the `Socket::connect()`
function.
Additionally the API of `Socket::connect()` was also reworked
according to the following boundary conditions requested by
libprocess maintainers:
* When libprocess is compiled without SSL support, neither the
declaration of the TLS configuration object nor the `connnect()`
overload that accepts the TLS configuration should be available.
* Passing just the servername is not an acceptable short-hand for
using the default TLS configuration together with that servername.
* When the incorrect overload is selected (i.e. passing TLS config
to a poll socket or omitting TLS configuration for a TLS socket),
the program should abort.
This following changes are introduced according to the requirements
above:
* A new class `openssl::TLSClientConfig` is introduced when libprocess
is compiled with ssl support.
* A new overload
`Socket::connect(const Address&, const TLSClientConfig&)` is
introduced when libprocess is compiled with ssl support.
* All call sites are adjusted to check the socket kind before calling
`connect()`.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
Diff: https://reviews.apache.org/r/70991/diff/2/
Changes: https://reviews.apache.org/r/70991/diff/1-2/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 70991: Updated `Socket::connect()` API according to
maintainer feedback.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70991/#review216342
-----------------------------------------------------------
Overall approach looks good, thanks for the update!
Some comments below, but I feel comfortable at this point with other reviewers looking things over.
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 26-28 (patched)
<https://reviews.apache.org/r/70991/#comment303515>
Stale comment? Connect requires the tls config, right?
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 62-63 (patched)
<https://reviews.apache.org/r/70991/#comment303516>
Some guidance on what these are and how one should use them would be helpful, probably on the variables here (or if on the types above, perhaps a comment here pointing the reader to the above explanations)
3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 82 (patched)
<https://reviews.apache.org/r/70991/#comment303517>
`LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME=libprocess` seems a little strange, can we call the "old" way `legacy` to signal that it's deprecated and we want to remove it?
3rdparty/libprocess/src/http.cpp
Line 1452 (original), 1454-1465 (patched)
<https://reviews.apache.org/r/70991/#comment303514>
How about:
```
Future<Nothing> connected = [&]() {
switch (scheme) {
case Scheme::HTTP:
return socket->connect(address);
#ifdef USE_SSL_SOCKET
case Scheme::HTTPS:
return socket->connect(
address,
network::openssl::create_tls_client_config(peer_hostname));
#endif
}
UNREACHABLE(); // If needed to silence compiler.
}();
```
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 518 (patched)
<https://reviews.apache.org/r/70991/#comment303513>
Ditto here w.r.t. EXIT
3rdparty/libprocess/src/posix/poll_socket.cpp
Lines 167 (patched)
<https://reviews.apache.org/r/70991/#comment303512>
EXIT will not produce a stack trace, can you use LOG(FATAL) here so that we can see who made the incorrect call?
Generally, we don't use EXIT for programming errors for this reason
3rdparty/libprocess/src/process.cpp
Lines 1453 (patched)
<https://reviews.apache.org/r/70991/#comment303511>
s/std:://
3rdparty/libprocess/src/tests/http_tests.cpp
Line 263 (original), 264-276 (patched)
<https://reviews.apache.org/r/70991/#comment303518>
Ditto here, can use the lambda approach?
3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 68 (patched)
<https://reviews.apache.org/r/70991/#comment303509>
no need for the break anymore?
3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 73 (patched)
<https://reviews.apache.org/r/70991/#comment303519>
Any comment on why we don't pass a hostname here or if we should? Do these tests even exercise the SSL socket? Will the branch be executed?
3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 75 (patched)
<https://reviews.apache.org/r/70991/#comment303510>
ditto here, no need for the break?
3rdparty/libprocess/src/tests/ssl_client.cpp
Line 147 (original), 147-157 (patched)
<https://reviews.apache.org/r/70991/#comment303520>
Ditto here for assignment from result of a lambda?
3rdparty/libprocess/src/tests/ssl_client.cpp
Lines 155 (patched)
<https://reviews.apache.org/r/70991/#comment303521>
Ditto here on a comment about why we don't pass hostname here
3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 559 (patched)
<https://reviews.apache.org/r/70991/#comment303523>
For all these spots passing None(), should we explain?
```
const Future<Nothing> connect = client.connect(
server_address.get(),
// E.g. Don't care about hostname certificate validation?
openssl::create_tls_client_config(None()));
```
3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 1008-1022 (patched)
<https://reviews.apache.org/r/70991/#comment303524>
Should we also verify the configure/verify error cases surfacing correctly as errors?
- Benjamin Mahler
On July 2, 2019, 5:50 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> -----------------------------------------------------------
>
> (Updated July 2, 2019, 5:50 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9878
> https://issues.apache.org/jira/browse/MESOS-9878
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reworked the API of `Socket::connect()` according to the following
> boundary conditions requested by libprocess maintainers:
>
> * It shall be possible to use custom connection options when
> connecting with a SSL socket.
> * When libprocess is compiled without SSL support, neither the
> declaration of the TLS configuration object nor the `connnect()`
> overload that accepts the TLS configuration should be available.
> * Passing just the servername is not an acceptable short-hand for
> using the default TLS configuration together with that servername.
> * When the incorrect overload is selected (i.e. passing TLS config
> to a poll socket or omitting TLS configuration for a TLS socket),
> the program should abort.
>
> This commit changes the API of `Socket::connect()` according to the
> requirements above. In particular:
>
> * A new class `openssl::TLSClientConfig` is introduced when libprocess
> is compiled with ssl support.
> * A new overload
> `Socket::connect(const Address&, const TLSClientConfig&)` is
> introduced when libprocess is compiled with ssl support.
> * All call sites are adjusted to check the socket kind before calling
> `connect()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am 1ddcc2d5a30f7bf3914138e497a9b228b515cd29
> 3rdparty/libprocess/include/process/socket.hpp 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83
> 3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION
> 3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
> 3rdparty/libprocess/src/poll_socket.hpp 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
> 3rdparty/libprocess/src/posix/poll_socket.cpp 74acb6942682a9d9626df81b303eba0a1c24ecf7
> 3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0
> 3rdparty/libprocess/src/tests/http_tests.cpp 4d372943a2d417d24d06444ec2e72909fb348017
> 3rdparty/libprocess/src/tests/socket_tests.cpp b09ae23a551c6587656b2d5f6f58c5267e8e0088
> 3rdparty/libprocess/src/tests/ssl_client.cpp de87b3b89c84d17f2ebba1f09e9ec682f139aace
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 5d360221937e68da185754f0633fa41a217c7107
>
>
> Diff: https://reviews.apache.org/r/70991/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>