You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/10/31 01:33:32 UTC
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/
-----------------------------------------------------------
(Updated Oct. 30, 2019, 6:33 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Factored out the handshake handling loop since it can be shared among a bunch of different `SSL_*` methods. And while I was at it, pulled in the code to implement `accept()` as well as `connect()`.
Summary (updated)
-----------------
SSL Wrapper: Implemented socket connection and handshake.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description (updated)
-------
This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.
Much of the setup and verification is taken verbatim from the
libevent socket implementation.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71665/diff/2/
Changes: https://reviews.apache.org/r/71665/diff/1-2/
Testing
-------
cmake --build . --target libprocess-tests
Successfully connected to Google :D
With something like this:
```
set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
});
Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
ASSERT_SOME(client);
AWAIT_ASSERT_READY(client->connect(
network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
openssl::create_tls_client_config(None())));
```
Thanks,
Joseph Wu
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review218813
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 369 (patched)
<https://reviews.apache.org/r/71665/#comment306698>
Unguarded `.get()`?
Also, s/inetAddress/inet_address/
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 411-412 (patched)
<https://reviews.apache.org/r/71665/#comment306699>
Fits on one line.
- Greg Mann
On Nov. 11, 2019, 7:40 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 7:40 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71665/diff/5/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review219046
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Dec. 16, 2019, 9:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2019, 9:58 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
> A change to the poll socket was necessary to prevent the SSL
> socket from holding a self-reference indefinitely.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
> 3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
>
>
> Diff: https://reviews.apache.org/r/71665/diff/7/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/
-----------------------------------------------------------
(Updated Dec. 16, 2019, 1:58 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Mostly commenting changes.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.
Much of the setup and verification is taken verbatim from the
libevent socket implementation.
A change to the poll socket was necessary to prevent the SSL
socket from holding a self-reference indefinitely.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
Diff: https://reviews.apache.org/r/71665/diff/7/
Changes: https://reviews.apache.org/r/71665/diff/6-7/
Testing
-------
cmake --build . --target libprocess-tests
Successfully connected to Google :D
With something like this:
```
set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
});
Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
ASSERT_SOME(client);
AWAIT_ASSERT_READY(client->connect(
network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
openssl::create_tls_client_config(None())));
```
Thanks,
Joseph Wu
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review219020
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 639-644 (patched)
<https://reviews.apache.org/r/71665/#comment307050>
Are you missing a `return Continue();` for the case where the send request doesn't exist?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 667-670 (patched)
<https://reviews.apache.org/r/71665/#comment307052>
Need to handle the case where `ERR_peek_error() != 0`?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 668-669 (patched)
<https://reviews.apache.org/r/71665/#comment307051>
Nit: fits on one line.
- Greg Mann
On Dec. 10, 2019, 11:53 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2019, 11:53 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
> A change to the poll socket was necessary to prevent the SSL
> socket from holding a self-reference indefinitely.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
> 3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
>
>
> Diff: https://reviews.apache.org/r/71665/diff/6/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 12, 2019, 4 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/openssl_socket.cpp
> > Lines 439 (patched)
> > <https://reviews.apache.org/r/71665/diff/6/?file=2183602#file2183602line439>
> >
> > Why not run these on the per-socket UPID also?
The UPID is added next patch. And we do replace this line with the `compute_thread` variable... although the value of `compute_thread == None()`.
> On Dec. 12, 2019, 4 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/openssl_socket.cpp
> > Lines 493-495 (patched)
> > <https://reviews.apache.org/r/71665/diff/6/?file=2183602#file2183602line493>
> >
> > Do you think we should add a timeout here to handle the case of a socket which doesn't complete the handshake? Maybe in a follow-up patch?
Added a TODO... and a JIRA: https://issues.apache.org/jira/browse/MESOS-10071
> On Dec. 12, 2019, 4 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/openssl_socket.cpp
> > Lines 549 (patched)
> > <https://reviews.apache.org/r/71665/diff/6/?file=2183602#file2183602line549>
> >
> > Since you create a UPID for the socket later, why not defer to that context here as well?
We don't use the `compute_thread` here because
1) `compute_thread` has type `Option<UPID>`, but there is no equivalent override for `defer`.
2) `compute_thread` will be `None` in this case, since this is a listening socket.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review219009
-----------------------------------------------------------
On Dec. 16, 2019, 1:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2019, 1:58 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
> A change to the poll socket was necessary to prevent the SSL
> socket from holding a self-reference indefinitely.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
> 3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
>
>
> Diff: https://reviews.apache.org/r/71665/diff/7/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review219009
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/openssl_socket.hpp
Lines 60 (patched)
<https://reviews.apache.org/r/71665/#comment307013>
Nit: s/should/must/ ?
3rdparty/libprocess/src/ssl/openssl_socket.hpp
Lines 71 (patched)
<https://reviews.apache.org/r/71665/#comment307014>
Nit: put arguments on different lines for consistency.
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Line 48 (original), 52 (patched)
<https://reviews.apache.org/r/71665/#comment307018>
Could you move these rename changes into the patch where the comments were originally added?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Line 285 (original), 295 (patched)
<https://reviews.apache.org/r/71665/#comment307041>
Nit: use `s` or `_s` consistently here and above.
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 329 (patched)
<https://reviews.apache.org/r/71665/#comment307045>
Could you add a comment here noting that the destructor is responsible for freeing this?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 433-434 (patched)
<https://reviews.apache.org/r/71665/#comment307022>
Nit: line wrapping should be updated.
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 436 (patched)
<https://reviews.apache.org/r/71665/#comment307019>
s/do we not/we do not/
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 439 (patched)
<https://reviews.apache.org/r/71665/#comment307020>
Why not run these on the per-socket UPID also?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 481 (patched)
<https://reviews.apache.org/r/71665/#comment307040>
s/openssl::Mode::SERVER/Mode::SERVER/
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 493-495 (patched)
<https://reviews.apache.org/r/71665/#comment307046>
Do you think we should add a timeout here to handle the case of a socket which doesn't complete the handshake? Maybe in a follow-up patch?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 495 (patched)
<https://reviews.apache.org/r/71665/#comment307021>
Is it correct that from this point on, we don't need to worry about calling `SSL_free()` because ownership of the context has been passed to the socket? If so, could you explain this in a comment? Also a comment in the header where `set_ssl_and_do_handshake` is declared?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 545 (patched)
<https://reviews.apache.org/r/71665/#comment307047>
Nit: s/socket/sockets/
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 548-556 (patched)
<https://reviews.apache.org/r/71665/#comment307049>
Could you make the newlines/indentation consistent across these two callbacks?
3rdparty/libprocess/src/ssl/openssl_socket.cpp
Lines 549 (patched)
<https://reviews.apache.org/r/71665/#comment307048>
Since you create a UPID for the socket later, why not defer to that context here as well?
- Greg Mann
On Dec. 10, 2019, 11:53 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2019, 11:53 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
> A change to the poll socket was necessary to prevent the SSL
> socket from holding a self-reference indefinitely.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
> 3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
>
>
> Diff: https://reviews.apache.org/r/71665/diff/6/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/
-----------------------------------------------------------
(Updated Dec. 10, 2019, 3:53 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Dealt with the libprocess socket manager deadlocking due to the multi-threaded accept of this socket.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description (updated)
-------
This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.
Much of the setup and verification is taken verbatim from the
libevent socket implementation.
A change to the poll socket was necessary to prevent the SSL
socket from holding a self-reference indefinitely.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
Diff: https://reviews.apache.org/r/71665/diff/6/
Changes: https://reviews.apache.org/r/71665/diff/5-6/
Testing
-------
cmake --build . --target libprocess-tests
Successfully connected to Google :D
With something like this:
```
set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
});
Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
ASSERT_SOME(client);
AWAIT_ASSERT_READY(client->connect(
network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
openssl::create_tls_client_config(None())));
```
Thanks,
Joseph Wu
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 449 (patched)
> > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line449>
> >
> > What do you mean "instead of in `accept`"? Aren't we in `accept` right now?
Oops, I originally put this comment in an override of `listen` instead.
> On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 460-464 (patched)
> > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line460>
> >
> > Is it possible to switch `PollSocketImpl::accept()` to use a weak reference instead of adding this code here?
Playing around with this a bit more, it may be necessary to make the change in PollSocketImpl. On Windows, there is no equivalent of `io::poll`, so I need to make the Windows poll socket into a weak pointer anyway.
We can limit the change of shared pointers into weak pointers to the `accept` and `connect` methods, since those are the two methods called by the SSL socket. But it wouldn't hurt to change all the poll socket's shared pointers into weak pointers (there might be a slight performance penalty associated with grabbing all these shared pointer locks though).
> On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 506 (patched)
> > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line506>
> >
> > What is the "configure callback"? Could you be more explicit/verbose here?
This terminology refers to this note in `libprocess/src/openssl.hpp`:
```
// Callback for setting SSL options after the TCP connection was
// established but before the TLS handshake has started.
Try<Nothing> configure_socket(
SSL* ssl,
Mode mode,
const Address& peer,
const Option<std::string>& peer_hostname);
```
This code was copied from the libevent SSL socket's accept logic too.
> On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 557-558 (patched)
> > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line557>
> >
> > Why vlog this failure case but not the others? Should we have logging for all these cases, or none?
This log was also copied from libevent. I can add more logging where it makes sense to.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review218881
-----------------------------------------------------------
On Nov. 11, 2019, 11:40 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 11:40 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71665/diff/5/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 3, 2019, 7:23 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp
> > Lines 506 (patched)
> > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line506>
> >
> > What is the "configure callback"? Could you be more explicit/verbose here?
>
> Joseph Wu wrote:
> This terminology refers to this note in `libprocess/src/openssl.hpp`:
> ```
> // Callback for setting SSL options after the TCP connection was
> // established but before the TLS handshake has started.
> Try<Nothing> configure_socket(
> SSL* ssl,
> Mode mode,
> const Address& peer,
> const Option<std::string>& peer_hostname);
> ```
>
> This code was copied from the libevent SSL socket's accept logic too.
It's still not clear to me what the "configure callback" is. Could you be more explicit/verbose in the comment? Perhaps refer to `configure_socket()` directly rather than calling it the "configure callback", or simply say "Right now, this function call does not do anything..."?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review218881
-----------------------------------------------------------
On Dec. 10, 2019, 11:53 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2019, 11:53 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
> A change to the poll socket was necessary to prevent the SSL
> socket from holding a self-reference indefinitely.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/posix/poll_socket.cpp ecc2bd492c4edd2f6ab0aae52d50bb3954881893
> 3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
> 3rdparty/libprocess/src/windows/poll_socket.cpp e2a84694ac554b4c23242fd93d93800c0334a943
>
>
> Diff: https://reviews.apache.org/r/71665/diff/6/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/#review218881
-----------------------------------------------------------
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 449 (patched)
<https://reviews.apache.org/r/71665/#comment306793>
What do you mean "instead of in `accept`"? Aren't we in `accept` right now?
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 460-464 (patched)
<https://reviews.apache.org/r/71665/#comment306795>
Is it possible to switch `PollSocketImpl::accept()` to use a weak reference instead of adding this code here?
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 502 (patched)
<https://reviews.apache.org/r/71665/#comment306796>
Nit: remove period at the end of failure message.
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 506 (patched)
<https://reviews.apache.org/r/71665/#comment306797>
What is the "configure callback"? Could you be more explicit/verbose here?
3rdparty/libprocess/src/ssl/socket_wrapper.cpp
Lines 557-558 (patched)
<https://reviews.apache.org/r/71665/#comment306798>
Why vlog this failure case but not the others? Should we have logging for all these cases, or none?
- Greg Mann
On Nov. 11, 2019, 7:40 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71665/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 7:40 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This fills in some of the SSL socket implementation,
> in particular the constructor, destructor, connect(),
> and accept() methods.
>
> Much of the setup and verification is taken verbatim from the
> libevent socket implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71665/diff/5/
>
>
> Testing
> -------
>
> cmake --build . --target libprocess-tests
>
> Successfully connected to Google :D
> With something like this:
> ```
> set_environment_variables({
> {"LIBPROCESS_SSL_ENABLED", "true"},
> {"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
> {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
> });
>
> Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
> ASSERT_SOME(client);
>
> AWAIT_ASSERT_READY(client->connect(
> network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
> openssl::create_tls_client_config(None())));
> ```
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/
-----------------------------------------------------------
(Updated Nov. 11, 2019, 11:40 a.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
* Forgot to initialize another bool... whoops.
* Added another boolean to the common SSL error handler, to deal with our API's expectations.
* Added a hack-ish thing to let the server socket destruct automatically.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.
Much of the setup and verification is taken verbatim from the
libevent socket implementation.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71665/diff/4/
Changes: https://reviews.apache.org/r/71665/diff/3-4/
Testing
-------
cmake --build . --target libprocess-tests
Successfully connected to Google :D
With something like this:
```
set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
});
Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
ASSERT_SOME(client);
AWAIT_ASSERT_READY(client->connect(
network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
openssl::create_tls_client_config(None())));
```
Thanks,
Joseph Wu
Re: Review Request 71665: SSL Wrapper: Implemented socket connection
and handshake.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71665/
-----------------------------------------------------------
(Updated Nov. 5, 2019, 5:55 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
* Changed lambdas to hold weak pointers to the socket wrapper.
* Added an accept queue and loop to deal with potential connections that never complete handshaking.
* Checked for EOF in the handshake loop.
Bugs: MESOS-10010
https://issues.apache.org/jira/browse/MESOS-10010
Repository: mesos
Description
-------
This fills in some of the SSL socket implementation,
in particular the constructor, destructor, connect(),
and accept() methods.
Much of the setup and verification is taken verbatim from the
libevent socket implementation.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 48860f8646d388685f0a60ad2a2f613b1f4be61a
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71665/diff/3/
Changes: https://reviews.apache.org/r/71665/diff/2-3/
Testing
-------
cmake --build . --target libprocess-tests
Successfully connected to Google :D
With something like this:
```
set_environment_variables({
{"LIBPROCESS_SSL_ENABLED", "true"},
{"LIBPROCESS_SSL_KEY_FILE", key_path().string()},
{"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()}
});
Try<Socket> client = Socket::create(SocketImpl::Kind::SSL);
ASSERT_SOME(client);
AWAIT_ASSERT_READY(client->connect(
network::inet::Address(net::IP::parse("216.58.194.206").get(), 443),
openssl::create_tls_client_config(None())));
```
Thanks,
Joseph Wu