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/23 19:37:02 UTC
Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/
-----------------------------------------------------------
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Bugs: MESOS-10009
https://issues.apache.org/jira/browse/MESOS-10009
Repository: mesos
Description
-------
This creates some new files implementing the SocketImpl class,
in preparation for implementing generic SSL sockets.
The new class is used whenever SSL is enabled, but Libevent is not.
Diffs
-----
3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71660/diff/1/
Testing
-------
cmake .. -ENABLE_SSL=1 # No Libevent here!
Thanks,
Joseph Wu
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/
-----------------------------------------------------------
(Updated Dec. 10, 2019, 3:50 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Renamed the new class from SSLSocketWrapper to OpenSSLSocketImpl.
The file was renamed too.
Bugs: MESOS-10009
https://issues.apache.org/jira/browse/MESOS-10009
Repository: mesos
Description
-------
This creates some new files implementing the SocketImpl class,
in preparation for implementing generic SSL sockets.
The new class is used whenever SSL is enabled, but Libevent is not.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e
3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
3rdparty/libprocess/src/openssl.cpp 8aab5acfb4bf3aa94625eaa7157714eca98940f7
3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
3rdparty/libprocess/src/ssl/openssl_socket.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/openssl_socket.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71660/diff/4/
Changes: https://reviews.apache.org/r/71660/diff/3-4/
Testing
-------
cmake .. -ENABLE_SSL=1 # No Libevent here!
Thanks,
Joseph Wu
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 19, 2019, 1:30 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 25 (patched)
> > <https://reviews.apache.org/r/71660/diff/3/?file=2173070#file2173070line25>
> >
> > This is mostly bike-shedding, so feel free to drop this issue if you disagree, but I'm not a huge fan of the name `SSLSocketWrapper`:
> >
> > First, the name would suggest to me that the constructor argument `int_fd s` is already a SSL socket that is being wrapped by this class.
> >
> > Second, the class doesn *wrap* a Socket, it *is* a SocketImpl.
> >
> > Finally, regarding the other implementations of `SocketImpl`, it seems like `OpenSSLSocketImpl` would be a bit more consistent.
>
> Joseph Wu wrote:
> I'm open to a rename. Greg/BenM, any opinions on a better name? (OpenSSLSocketImpl sounds fine to me.)
>
> One of my earlier iterations mooted the possibility of making this class an actual wrapper (like a Python SSL wrapper), which is why I named it this way.
`OpenSSLSocketImpl` sounds good to me.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218670
-----------------------------------------------------------
On Nov. 13, 2019, 7:03 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2019, 7:03 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
>
> The new class is used whenever SSL is enabled, but Libevent is not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e
> 3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
> 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71660/diff/3/
>
>
> Testing
> -------
>
> cmake .. -ENABLE_SSL=1 # No Libevent here!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218670
-----------------------------------------------------------
3rdparty/libprocess/src/CMakeLists.txt
Lines 23 (patched)
<https://reviews.apache.org/r/71660/#comment306536>
I'm not sure I completely understand the rationale for the exclusivity here (and elsewhere in the review). Intuitively, I'd expect
```
OpenSSL >= 1.1 found => Enable SocketWrapper
Libevent enabled => Enable LibeventSSLSocket
--enable-ssl => Require either of the above to be enabled
```
In particular it doesn't seem to be too far-fetched that someone might want to use libevent but not the libevent ssl socket in the future, due to bugs like MESOS-9867.
3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 24 (patched)
<https://reviews.apache.org/r/71660/#comment306537>
Why is this derived from `PollSocketImpl`, rather than just from `SocketImpl`?
3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 25 (patched)
<https://reviews.apache.org/r/71660/#comment306539>
This is mostly bike-shedding, so feel free to drop this issue if you disagree, but I'm not a huge fan of the name `SSLSocketWrapper`:
First, the name would suggest to me that the constructor argument `int_fd s` is already a SSL socket that is being wrapped by this class.
Second, the class doesn *wrap* a Socket, it *is* a SocketImpl.
Finally, regarding the other implementations of `SocketImpl`, it seems like `OpenSSLSocketImpl` would be a bit more consistent.
- Benno Evers
On Nov. 13, 2019, 7:03 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2019, 7:03 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
>
> The new class is used whenever SSL is enabled, but Libevent is not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e
> 3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
> 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71660/diff/3/
>
>
> Testing
> -------
>
> cmake .. -ENABLE_SSL=1 # No Libevent here!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/
-----------------------------------------------------------
(Updated Nov. 13, 2019, 11:03 a.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Modified autotools build too.
Bugs: MESOS-10009
https://issues.apache.org/jira/browse/MESOS-10009
Repository: mesos
Description
-------
This creates some new files implementing the SocketImpl class,
in preparation for implementing generic SSL sockets.
The new class is used whenever SSL is enabled, but Libevent is not.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 641251af5631e9b3928dfb282cdbc266ba76572e
3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71660/diff/3/
Changes: https://reviews.apache.org/r/71660/diff/2-3/
Testing
-------
cmake .. -ENABLE_SSL=1 # No Libevent here!
Thanks,
Joseph Wu
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 8, 2019, 12:33 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 24-28 (patched)
> > <https://reviews.apache.org/r/71660/diff/2/?file=2170109#file2170109line24>
> >
> > What do you think about making `SSLSocketWrapper` a true wrapper of the underlying socket? In other words, let's make the wrapper a subclass of `SocketImpl` and have it hold a `SocketImpl` member which is the underlying socket that it's using for communication. We could add a `create()` method and a constructor like this:
> > ```
> > Try<std::shared_ptr<SocketImpl>> SSLSocketWrapper::create(const SocketImpl& socketImpl_)
> > {
> > openssl::initialize();
> >
> > if (!openssl::flags().enabled) {
> > return Error("SSL is disabled");
> > }
> >
> > return std::make_shared<SSLSocketWrapper>(socketImpl_);
> > }
> >
> >
> > SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_)
> > : socketImpl(socketImpl_),
> > ssl(nullptr) {}
> > ```
> >
> > then we could call methods like `connect()` and `accept()` directly on the held socket within the wrapper code.
> >
> > It's a small structural change, but I think it would help to communicate to readers how the underlying socket and the SSL wrapper are related. Further, it makes it more obvious how we could evolve the wrapper class into some kind of `Server` and `Client` objects which are initialized with a socket.
> >
> > WDYT?
The `SocketWrapperBIOData` could also hold a `SocketImpl` member instead of an `int_fd`, right?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218573
-----------------------------------------------------------
On Oct. 24, 2019, 1:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2019, 1:41 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
>
> The new class is used whenever SSL is enabled, but Libevent is not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
> 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71660/diff/2/
>
>
> Testing
> -------
>
> cmake .. -ENABLE_SSL=1 # No Libevent here!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 7, 2019, 4:33 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 24-28 (patched)
> > <https://reviews.apache.org/r/71660/diff/2/?file=2170109#file2170109line24>
> >
> > What do you think about making `SSLSocketWrapper` a true wrapper of the underlying socket? In other words, let's make the wrapper a subclass of `SocketImpl` and have it hold a `SocketImpl` member which is the underlying socket that it's using for communication. We could add a `create()` method and a constructor like this:
> > ```
> > Try<std::shared_ptr<SocketImpl>> SSLSocketWrapper::create(const SocketImpl& socketImpl_)
> > {
> > openssl::initialize();
> >
> > if (!openssl::flags().enabled) {
> > return Error("SSL is disabled");
> > }
> >
> > return std::make_shared<SSLSocketWrapper>(socketImpl_);
> > }
> >
> >
> > SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_)
> > : socketImpl(socketImpl_),
> > ssl(nullptr) {}
> > ```
> >
> > then we could call methods like `connect()` and `accept()` directly on the held socket within the wrapper code.
> >
> > It's a small structural change, but I think it would help to communicate to readers how the underlying socket and the SSL wrapper are related. Further, it makes it more obvious how we could evolve the wrapper class into some kind of `Server` and `Client` objects which are initialized with a socket.
> >
> > WDYT?
>
> Greg Mann wrote:
> The `SocketWrapperBIOData` could also hold a `SocketImpl` member instead of an `int_fd`, right?
The main implementation hurdle I see from making `SSLSocketWrapper` hold a `SocketImpl` member, is that the `SSLSocketWrapper` itself then cannot easily be a subclass of `SocketImpl`. When I tried to add a `PollSocketImpl` member variable, the underlying socket `int_fd` ownership became obscure, since both the SSLSocketWrapper and the PollSocketImpl need to own the `int_fd` in order to be useful.
Having the `SSLSocketWrapper` subclass a `SocketImpl` is useful for all the areas that use Socket objects. This obstraction lets us parameterize tons of our tests for SSL and non-SSL.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218573
-----------------------------------------------------------
On Oct. 23, 2019, 6:41 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2019, 6:41 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
>
> The new class is used whenever SSL is enabled, but Libevent is not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
> 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71660/diff/2/
>
>
> Testing
> -------
>
> cmake .. -ENABLE_SSL=1 # No Libevent here!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/#review218573
-----------------------------------------------------------
3rdparty/libprocess/src/CMakeLists.txt
Lines 101-103 (patched)
<https://reviews.apache.org/r/71660/#comment306309>
Need to update autotools build also? Or is this Windows-only for now?
3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 24-28 (patched)
<https://reviews.apache.org/r/71660/#comment306310>
What do you think about making `SSLSocketWrapper` a true wrapper of the underlying socket? In other words, let's make the wrapper a subclass of `SocketImpl` and have it hold a `SocketImpl` member which is the underlying socket that it's using for communication. We could add a `create()` method and a constructor like this:
```
Try<std::shared_ptr<SocketImpl>> SSLSocketWrapper::create(const SocketImpl& socketImpl_)
{
openssl::initialize();
if (!openssl::flags().enabled) {
return Error("SSL is disabled");
}
return std::make_shared<SSLSocketWrapper>(socketImpl_);
}
SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_)
: socketImpl(socketImpl_),
ssl(nullptr) {}
```
then we could call methods like `connect()` and `accept()` directly on the held socket within the wrapper code.
It's a small structural change, but I think it would help to communicate to readers how the underlying socket and the SSL wrapper are related. Further, it makes it more obvious how we could evolve the wrapper class into some kind of `Server` and `Client` objects which are initialized with a socket.
WDYT?
- Greg Mann
On Oct. 24, 2019, 1:41 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2019, 1:41 a.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
>
> The new class is used whenever SSL is enabled, but Libevent is not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
> 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
> 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
> 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71660/diff/2/
>
>
> Testing
> -------
>
> cmake .. -ENABLE_SSL=1 # No Libevent here!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71660/
-----------------------------------------------------------
(Updated Oct. 23, 2019, 6:41 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till Toenshoff.
Changes
-------
Removed the `listen` override, since we can just use the PollSocket's implementation here.
Bugs: MESOS-10009
https://issues.apache.org/jira/browse/MESOS-10009
Repository: mesos
Description
-------
This creates some new files implementing the SocketImpl class,
in preparation for implementing generic SSL sockets.
The new class is used whenever SSL is enabled, but Libevent is not.
Diffs (updated)
-----
3rdparty/libprocess/src/CMakeLists.txt 40c8ef935104bab4ea2f8b2b5919450b55165f60
3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135
3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION
3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71660/diff/2/
Changes: https://reviews.apache.org/r/71660/diff/1-2/
Testing
-------
cmake .. -ENABLE_SSL=1 # No Libevent here!
Thanks,
Joseph Wu