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/07/21 23:38:23 UTC
Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration flag
for SSLv2.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3121
https://issues.apache.org/jira/browse/MESOS-3121
Repository: mesos
Description
-------
Removed 'SSL_ENABLE_SSL_V2' configuration flag for SSLv2.
Diffs
-----
3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36656/diff/
Testing
-------
make check.
Thanks,
Joris Van Remoortere
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Michael Park <mc...@gmail.com>.
> On July 21, 2015, 10:59 p.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 676
> > <https://reviews.apache.org/r/36656/diff/1/?file=1018171#file1018171line676>
> >
> > s/protocol/protocols/
Now that we're only referring to SSL-V3, I think it should be:
`s/conditionally test these protocol/conditionally test for this protocol/`
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/#review92492
-----------------------------------------------------------
On July 21, 2015, 10:51 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36656/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Michael Park.
>
>
> Bugs: MESOS-3121
> https://issues.apache.org/jira/browse/MESOS-3121
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
> 3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36656/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/#review92492
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 673)
<https://reviews.apache.org/r/36656/#comment146704>
Since it's used in several places, will it make sense to have a helper function that returns a vector with protocols?
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 675)
<https://reviews.apache.org/r/36656/#comment146699>
s/protocol/protocols/
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 807)
<https://reviews.apache.org/r/36656/#comment146700>
ditto.
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 865)
<https://reviews.apache.org/r/36656/#comment146701>
ditto.
- Artem Harutyunyan
On July 21, 2015, 3:51 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36656/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 3:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Michael Park.
>
>
> Bugs: MESOS-3121
> https://issues.apache.org/jira/browse/MESOS-3121
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
> 3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36656/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On July 22, 2015, 7:51 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/openssl.cpp, line 332
> > <https://reviews.apache.org/r/36656/diff/1/?file=1018170#file1018170line332>
> >
> > Saw this while grepping for `SSLv2`.
> >
> > We still need to use this rather than `TLS_method` currently because of support for older OpenSSL, correct?
> >
> > Just wondering since we no longer support `SSLv2`. OpenSSL documentation [1] mentions that `SSLv23_method` has been replaced with `TLS_method` which supports `SSLv3`, `TLSv1`, `TLSv1.1` and `TLSv1.2`. It also mentioned that `TLS_method` was introduced in OpenSSL 1.1.0.
> >
> > [1] https://www.openssl.org/docs/ssl/TLSv1_1_server_method.html
Indeed. I think we should maintain support for older versions of OpenSSL for now.
I will add a comment referencing this for future deprecation though. Thanks!
> On July 22, 2015, 7:51 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 483-489
> > <https://reviews.apache.org/r/36656/diff/1/?file=1018170#file1018170line483>
> >
> > Saw this while grepping for `SSLv2`.
> >
> > I feel like `SSL_CTX_clear_options(ctx, SSL_CTX_get_options(ctx));` would be a concise way to do this?
> >
> > It would also continue to completely clear the options, even if new protocols were to be introduced into OpenSSL.
I agree that it would be more concise; however, it would also clear the options that are set by default during construction. I don't think it is a safe strategy to blindly clear them.
I'm going to leave the manual clear for now.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/#review92638
-----------------------------------------------------------
On July 21, 2015, 10:51 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36656/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Michael Park.
>
>
> Bugs: MESOS-3121
> https://issues.apache.org/jira/browse/MESOS-3121
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
> 3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36656/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/#review92638
-----------------------------------------------------------
Ship it!
+ Comments from Artem and BenH.
3rdparty/libprocess/src/openssl.cpp (line 327)
<https://reviews.apache.org/r/36656/#comment146860>
Saw this while grepping for `SSLv2`.
We still need to use this rather than `TLS_method` currently because of support for older OpenSSL, correct?
Just wondering since we no longer support `SSLv2`. OpenSSL documentation [1] mentions that `SSLv23_method` has been replaced with `TLS_method` which supports `SSLv3`, `TLSv1`, `TLSv1.1` and `TLSv1.2`. It also mentioned that `TLS_method` was introduced in OpenSSL 1.1.0.
[1] https://www.openssl.org/docs/ssl/TLSv1_1_server_method.html
3rdparty/libprocess/src/openssl.cpp (lines 478 - 484)
<https://reviews.apache.org/r/36656/#comment146859>
Saw this while grepping for `SSLv2`.
I feel like `SSL_CTX_clear_options(ctx, SSL_CTX_get_options(ctx));` would be a concise way to do this?
It would also continue to completely clear the options, even if new protocols were to be introduced into OpenSSL.
- Michael Park
On July 21, 2015, 10:51 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36656/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Michael Park.
>
>
> Bugs: MESOS-3121
> https://issues.apache.org/jira/browse/MESOS-3121
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
> 3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36656/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/
-----------------------------------------------------------
(Updated July 26, 2015, 5:28 p.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Changes
-------
Addressed comments.
Bugs: MESOS-3121
https://issues.apache.org/jira/browse/MESOS-3121
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36656/diff/
Testing
-------
make check.
Thanks,
Joris Van Remoortere
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/#review92534
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/openssl.cpp (line 489)
<https://reviews.apache.org/r/36656/#comment146739>
Can we leave a longer comment as to why we do this? Maybe link to something if that's helpful? Maybe even include a quick comment where we define the flags why we specifically omit v2 there as well? It'll be helpful to leave a nice trail of breadcrumbs so someone else doesn't come and try and re-add it.
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 674)
<https://reviews.apache.org/r/36656/#comment146740>
We missed some! s/openssl/OpenSSL/ ;-) Here and throughout please.
- Benjamin Hindman
On July 21, 2015, 10:51 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36656/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Michael Park.
>
>
> Bugs: MESOS-3121
> https://issues.apache.org/jira/browse/MESOS-3121
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
> 3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36656/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 36656: Removed 'SSL_ENABLE_SSL_V2' configuration
flag for SSLv2.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36656/
-----------------------------------------------------------
(Updated July 21, 2015, 10:51 p.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3121
https://issues.apache.org/jira/browse/MESOS-3121
Repository: mesos
Description (updated)
-------
See summary.
Diffs
-----
3rdparty/libprocess/src/openssl.hpp 3f8d351589f8bb26c886da12c53b5e02a242376a
3rdparty/libprocess/src/openssl.cpp 6aa2a4db8d64011d0fde6ff0cf4b144c41949d39
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36656/diff/
Testing
-------
make check.
Thanks,
Joris Van Remoortere