You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2019/11/08 00:33:13 UTC
Re: 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/#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 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
>
>