Re: Review Request 70991: Added ability to pass custom SSL context to `Socket::connect()`.

2019-07-05 Thread Benno Evers

---
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()`.

2019-07-04 Thread Till Toenshoff via Review Board


> On July 4, 2019, 11:53 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 806 (original), 807 (patched)
> > 
> >
> > 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()`.

2019-07-04 Thread Till Toenshoff via Review Board

---
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)


```
 (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)


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)


s/libprocess/legacy/



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


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()`.

2019-07-04 Thread Benno Evers

---
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