mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Date Tue, 12 Nov 2019 00:00:40 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message