mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.
Date Fri, 08 Nov 2019 00:33:13 GMT

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


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