You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2015/06/29 03:00:58 UTC

Re: Review Request 31207: WIP: Support for SSL and non-ssl traffic simultaneously.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31207/
-----------------------------------------------------------

(Updated June 29, 2015, 1 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Added test cases.
Added comments around msg_peek check.


Bugs: MESOS-2085
    https://issues.apache.org/jira/browse/MESOS-2085


Repository: mesos


Description
-------

Add a flag SSL_SUPPORT_DOWNGRADE which allows:
1. an SSL accepting socket to peek at the incoming data. If the hello handshake bits are not set, then accept as a Socket::POLL socket instead.
2. When calling Process::link or Process:send(Message), if a new connection is required, allow a second attempt using Socket::POLL if an SSL connection was first attempted.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/socket.hpp f53d2e1dbb31e135c8951145d379cbbff3044448 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 4f2cd357bfdb5268d2bae2df5d7138ff14064bf6 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 2920e0e1a5643118b14911d77fb682e60958b4e6 
  3rdparty/libprocess/src/openssl.hpp 60c7b078b891e09d53d82508bb2965addf359d68 
  3rdparty/libprocess/src/openssl.cpp 6ff4adb4c9792ff10d8c6ed2f3b2f3d8d0d7f1a8 
  3rdparty/libprocess/src/poll_socket.hpp 553aa641525d587a44608d7c6c4f16b09b47cfe0 
  3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e 
  3rdparty/libprocess/src/tests/ssl_tests.cpp c077aaeaecbe2cdcdad2b042741eeb8906699a22 

Diff: https://reviews.apache.org/r/31207/diff/


Testing
-------

Running with:
1) An SSL master
  - connect a non-ssl slave
  - connect a non-ssl framework
  - connect an ssl slave
  - connect an ssl framework
2) A non-ssl master
  - connect a non-ssl slave
  - connect a non-ssl framework
  - connect an ssl slave
  - connect an ssl framework


Thanks,

Joris Van Remoortere

Re: Review Request 31207: Support for SSL and non-ssl traffic simultaneously.

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31207/#review89698
-----------------------------------------------------------


This is an incomplete review but I wanted to publish it sot that we can deal with the 'link_connect' and 'send_connect' issue.


3rdparty/libprocess/src/openssl.cpp (line 336)
<https://reviews.apache.org/r/31207/#comment142369>

    s/plain HTTP/a non-SSL socket/
    
    Because someone using the socket might not want to use it for HTTP! ;-) Only for libprocess will we expect the data to be HTTP.
    
    Feel free to check as much in process::initialize and print out a big warning there too?



3rdparty/libprocess/src/process.cpp (line 1245)
<https://reviews.apache.org/r/31207/#comment142371>

    How do you know that the ProcessBase pointer is still going to be valid across this async callback? A ProcessReference here might be better suited.



3rdparty/libprocess/src/process.cpp (lines 1263 - 1264)
<https://reviews.apache.org/r/31207/#comment142370>

    We can't "close this socket" yet because it will generate "exited" events as though this link was unsuccessful.
    
    One suggestion for now is to add a 'bool generate_exited' parameter to SocketManager::close that only generates the exited events if true (the default).
    
    This general pattern of "closing" the socket is still unfortunate, however, because it'll mean we may queue up some data to send (Encoders) that we then close even though the downgraded connection may work correctly. This could start to introduce some funky things in practice, as in, some messages actually look like they're getting dropped! So the real way to fix this is to probably grab the SocketManager mutex and actually update all of the data structures with the "new" socket. Or alternatively, could we swap the "implementation" of a Socket and reuse the same file descriptor?


- Benjamin Hindman


On June 29, 2015, 1:22 a.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31207/
> -----------------------------------------------------------
> 
> (Updated June 29, 2015, 1:22 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2085
>     https://issues.apache.org/jira/browse/MESOS-2085
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add a flag SSL_SUPPORT_DOWNGRADE which allows:
> 1. an SSL accepting socket to peek at the incoming data. If the hello handshake bits are not set, then accept as a Socket::POLL socket instead.
> 2. When calling Process::link or Process:send(Message), if a new connection is required, allow a second attempt using Socket::POLL if an SSL connection was first attempted.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp f53d2e1dbb31e135c8951145d379cbbff3044448 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 4f2cd357bfdb5268d2bae2df5d7138ff14064bf6 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 2920e0e1a5643118b14911d77fb682e60958b4e6 
>   3rdparty/libprocess/src/openssl.hpp 60c7b078b891e09d53d82508bb2965addf359d68 
>   3rdparty/libprocess/src/openssl.cpp 6ff4adb4c9792ff10d8c6ed2f3b2f3d8d0d7f1a8 
>   3rdparty/libprocess/src/poll_socket.hpp 553aa641525d587a44608d7c6c4f16b09b47cfe0 
>   3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp c077aaeaecbe2cdcdad2b042741eeb8906699a22 
> 
> Diff: https://reviews.apache.org/r/31207/diff/
> 
> 
> Testing
> -------
> 
> Running with:
> 1) An SSL master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 2) A non-ssl master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 31207: Support for SSL and non-ssl traffic simultaneously.

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31207/#review89700
-----------------------------------------------------------



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 786)
<https://reviews.apache.org/r/31207/#comment142375>

    Could this ever not be EV_READ? Can we make this stricter as a CHECK instead?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 789 - 790)
<https://reviews.apache.org/r/31207/#comment142373>

    s/buf/data/
    s/int ret/ssize_t size/



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 855)
<https://reviews.apache.org/r/31207/#comment142374>

    request->ready_event = event_new(
        ...
    );
    
    Also, I'd like to suggest matching the event name with the callback, i.e., 'peek_event' and 'peek_callback' or 'peek_ready_event' and 'peek_ready_callback'.
    
    Finally, I so wish we could just do:
    
    io::peek(request->socket, 6)
      .then([request](const string& data) {
        // Comment about the rules ...
        if (data.length() < 2) { // Rule 1
        
        } else if (...) { // Rule 2.
        
        } else if (...) { // Rule 3.
        
        }
        
        if (ssl) {
          accept_SSL_callback(request);
        } else {
          ...;
        }
      });


- Benjamin Hindman


On June 29, 2015, 1:22 a.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31207/
> -----------------------------------------------------------
> 
> (Updated June 29, 2015, 1:22 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2085
>     https://issues.apache.org/jira/browse/MESOS-2085
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add a flag SSL_SUPPORT_DOWNGRADE which allows:
> 1. an SSL accepting socket to peek at the incoming data. If the hello handshake bits are not set, then accept as a Socket::POLL socket instead.
> 2. When calling Process::link or Process:send(Message), if a new connection is required, allow a second attempt using Socket::POLL if an SSL connection was first attempted.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp f53d2e1dbb31e135c8951145d379cbbff3044448 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 4f2cd357bfdb5268d2bae2df5d7138ff14064bf6 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 2920e0e1a5643118b14911d77fb682e60958b4e6 
>   3rdparty/libprocess/src/openssl.hpp 60c7b078b891e09d53d82508bb2965addf359d68 
>   3rdparty/libprocess/src/openssl.cpp 6ff4adb4c9792ff10d8c6ed2f3b2f3d8d0d7f1a8 
>   3rdparty/libprocess/src/poll_socket.hpp 553aa641525d587a44608d7c6c4f16b09b47cfe0 
>   3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp c077aaeaecbe2cdcdad2b042741eeb8906699a22 
> 
> Diff: https://reviews.apache.org/r/31207/diff/
> 
> 
> Testing
> -------
> 
> Running with:
> 1) An SSL master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 2) A non-ssl master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 31207: Support for SSL and non-ssl traffic simultaneously.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31207/#review89710
-----------------------------------------------------------


Patch looks great!

Reviews applied: [29526, 29527, 29528, 29529, 29530, 29531, 29532, 29533, 29534, 29535, 29536, 29538, 29539, 29540, 29569, 28763, 29406, 31207]

All tests passed.

- Mesos ReviewBot


On June 29, 2015, 6:36 a.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31207/
> -----------------------------------------------------------
> 
> (Updated June 29, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-2085
>     https://issues.apache.org/jira/browse/MESOS-2085
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add a flag SSL_SUPPORT_DOWNGRADE which allows:
> 1. an SSL accepting socket to peek at the incoming data. If the hello handshake bits are not set, then accept as a Socket::POLL socket instead.
> 2. When calling Process::link or Process:send(Message), if a new connection is required, allow a second attempt using Socket::POLL if an SSL connection was first attempted.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp f53d2e1dbb31e135c8951145d379cbbff3044448 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 4f2cd357bfdb5268d2bae2df5d7138ff14064bf6 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 2920e0e1a5643118b14911d77fb682e60958b4e6 
>   3rdparty/libprocess/src/openssl.hpp 60c7b078b891e09d53d82508bb2965addf359d68 
>   3rdparty/libprocess/src/openssl.cpp 6ff4adb4c9792ff10d8c6ed2f3b2f3d8d0d7f1a8 
>   3rdparty/libprocess/src/poll_socket.hpp 553aa641525d587a44608d7c6c4f16b09b47cfe0 
>   3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp c077aaeaecbe2cdcdad2b042741eeb8906699a22 
> 
> Diff: https://reviews.apache.org/r/31207/diff/
> 
> 
> Testing
> -------
> 
> Running with:
> 1) An SSL master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 2) A non-ssl master
>   - connect a non-ssl slave
>   - connect a non-ssl framework
>   - connect an ssl slave
>   - connect an ssl framework
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>