You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/06/20 17:48:39 UTC
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/
-----------------------------------------------------------
(Updated June 20, 2019, 5:48 p.m.)
Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
Changes
-------
Renamed algorithm -> scheme
Summary (updated)
-----------------
Introduced optional new scheme for hostname validation.
Bugs: MESOS-9809
https://issues.apache.org/jira/browse/MESOS-9809
Repository: mesos
Description (updated)
-------
This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the
As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.
See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.
Diffs (updated)
-----
3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
Diff: https://reviews.apache.org/r/70749/diff/4/
Changes: https://reviews.apache.org/r/70749/diff/3-4/
Testing
-------
Todo!
Thanks,
Benno Evers
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
Posted by Benno Evers <be...@mesosphere.com>.
> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 808 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line808>
> >
> > I know you just moved it, but where do these 100ms come from and how could we be more explicit about that choice?
> >
> > I would suggest to use a const with some explaining comment how that value was chosen - can we please? :D
I'm afraid I have no idea where the 100ms come from. The suggestion sounds good, but I think the changes will fit better in a separate review, since they're not really related to hostname validation.
> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 984-985 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line984>
> >
> > This sounds like a great idea worth spending 1 more cycle on -- can we create and reference a ticket that explains this jazz as nicely as we do here in the code?
> >
> > My thought is that being open about this idea in JIRA, we would provide more chances of getting community support for it.
Opened https://issues.apache.org/jira/browse/MESOS-9855
> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 989 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line989>
> >
> > Shall we explain why this is the `right` way - aka best practices?
Huh, I actually removed this now: I originally had a look at the OpenSSL example code and at RFC6125, but missed that partial wildcards are only disallowed in *internationalized* domain names. (and for these, openssl already does the correct thing regardless of this flag.)
With this removed, we're a bit more loose than what a web browser would accept, but Mesos is not a web browser so that seems fine.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/#review216017
-----------------------------------------------------------
On June 20, 2019, 5:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 5:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the handshake.
>
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
>
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
>
>
> Diff: https://reviews.apache.org/r/70749/diff/4/
>
>
> Testing
> -------
>
> Todo!
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
Posted by Benno Evers <be...@mesosphere.com>.
> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 808 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line808>
> >
> > I know you just moved it, but where do these 100ms come from and how could we be more explicit about that choice?
> >
> > I would suggest to use a const with some explaining comment how that value was chosen - can we please? :D
>
> Benno Evers wrote:
> I'm afraid I have no idea where the 100ms come from. The suggestion sounds good, but I think the changes will fit better in a separate review, since they're not really related to hostname validation.
Fixed in https://reviews.apache.org/r/70933/
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/#review216017
-----------------------------------------------------------
On June 21, 2019, 3:32 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> -----------------------------------------------------------
>
> (Updated June 21, 2019, 3:32 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the
>
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
>
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
>
>
> Diff: https://reviews.apache.org/r/70749/diff/5/
>
>
> Testing
> -------
>
> Todo!
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/#review216017
-----------------------------------------------------------
3rdparty/libprocess/src/openssl.cpp
Lines 147 (patched)
<https://reviews.apache.org/r/70749/#comment302961>
s/algorithm/scheme/?
3rdparty/libprocess/src/openssl.cpp
Lines 153 (patched)
<https://reviews.apache.org/r/70749/#comment302962>
s/algorithm/scheme/?
Also, we should tell the user which version he needs at least here, nice touch ;)
3rdparty/libprocess/src/openssl.cpp
Lines 565-566 (patched)
<https://reviews.apache.org/r/70749/#comment302963>
Should we
```
EXIT(EXIT_FAILURE)
<< "Unknown value for hostname_validation_scheme: "
<< ssl_flags->hostname_validation_scheme;
```
instead here and below to be perfectly explicit and consistent?
3rdparty/libprocess/src/openssl.cpp
Lines 576 (patched)
<https://reviews.apache.org/r/70749/#comment302964>
s/algorithm/scheme/?
3rdparty/libprocess/src/openssl.cpp
Line 755 (original), 788 (patched)
<https://reviews.apache.org/r/70749/#comment302965>
Maybe
```
// When using the OpenSSL built-in scheme,...
```
3rdparty/libprocess/src/openssl.cpp
Lines 795 (patched)
<https://reviews.apache.org/r/70749/#comment302966>
s/algorithm/scheme/
3rdparty/libprocess/src/openssl.cpp
Lines 808 (patched)
<https://reviews.apache.org/r/70749/#comment302967>
I know you just moved it, but where do these 100ms come from and how could we be more explicit about that choice?
I would suggest to use a const with some explaining comment how that value was chosen - can we please? :D
3rdparty/libprocess/src/openssl.cpp
Lines 984-985 (patched)
<https://reviews.apache.org/r/70749/#comment302968>
This sounds like a great idea worth spending 1 more cycle on -- can we create and reference a ticket that explains this jazz as nicely as we do here in the code?
My thought is that being open about this idea in JIRA, we would provide more chances of getting community support for it.
3rdparty/libprocess/src/openssl.cpp
Lines 989 (patched)
<https://reviews.apache.org/r/70749/#comment302969>
Shall we explain why this is the `right` way - aka best practices?
3rdparty/libprocess/src/openssl.cpp
Lines 1016 (patched)
<https://reviews.apache.org/r/70749/#comment302970>
s/ip/IP/
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 444-448 (patched)
<https://reviews.apache.org/r/70749/#comment302999>
Great comment - thanks!
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 569 (patched)
<https://reviews.apache.org/r/70749/#comment303000>
Now that we log the `peer_hostname`, I wonder if it made sense to also log the `peer_ip` like this - what do you think? If you agree, then we should even try to render a single log-line out of those two - it will be noisy enough on level 2.
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 1129 (patched)
<https://reviews.apache.org/r/70749/#comment303002>
I feel `peername` comes in a bit surprising here if one does not know about `getpeername()`.
Maybe s/peername/IP address/?
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 1136 (patched)
<https://reviews.apache.org/r/70749/#comment303001>
As mentioned in some other place of this chain, let's add a neat JIRA on this plan.
- Till Toenshoff
On June 20, 2019, 5:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 5:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the handshake.
>
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
>
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
>
>
> Diff: https://reviews.apache.org/r/70749/diff/4/
>
>
> Testing
> -------
>
> Todo!
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
Posted by Jan-Philip Gehrcke via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/#review215999
-----------------------------------------------------------
Fix it, then Ship it!
Just saw that I did not yet submit this review, it's from yesterday's conversation. Hope it still applies. Feel free to ignore.
3rdparty/libprocess/src/openssl.cpp
Lines 563 (patched)
<https://reviews.apache.org/r/70749/#comment302937>
Let's maybe emit a warning when VERIFY_CERT is set to true, but ALLOW_DOWNGRADE is also set to true
- Jan-Philip Gehrcke
On June 20, 2019, 5:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 5:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the handshake.
>
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
>
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
> 3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
> 3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
> 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
>
>
> Diff: https://reviews.apache.org/r/70749/diff/4/
>
>
> Testing
> -------
>
> Todo!
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70749: Introduced RFC6125-compliant hostname
validation scheme.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/
-----------------------------------------------------------
(Updated July 4, 2019, 6:52 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
Changes
-------
Renamed 'libprocess' -> 'legacy'
Bugs: MESOS-9809
https://issues.apache.org/jira/browse/MESOS-9809
Repository: mesos
Description (updated)
-------
This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be set to 'legacy'
to select the previous hostname validation behaviour or to
'openssl' to use standardized OpenSSL algorithms to handle
hostname validation as part of the TLS handshake.
As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.
See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.
Diffs (updated)
-----
3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
Diff: https://reviews.apache.org/r/70749/diff/9/
Changes: https://reviews.apache.org/r/70749/diff/8-9/
Testing
-------
See added unit tests later in this chain.
Thanks,
Benno Evers
Re: Review Request 70749: Introduced RFC6125-compliant hostname
validation scheme.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/
-----------------------------------------------------------
(Updated July 2, 2019, 5:49 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
Changes
-------
Rebased onto latest master; changed title.
Summary (updated)
-----------------
Introduced RFC6125-compliant hostname validation scheme.
Bugs: MESOS-9809
https://issues.apache.org/jira/browse/MESOS-9809
Repository: mesos
Description
-------
This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the
As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.
See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.
Diffs (updated)
-----
3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 7e2229a9ed815727500bd457356e5531607fa6cf
Diff: https://reviews.apache.org/r/70749/diff/8/
Changes: https://reviews.apache.org/r/70749/diff/7-8/
Testing (updated)
-------
See added unit tests later in this chain.
Thanks,
Benno Evers
Re: Review Request 70749: Introduced optional new scheme for hostname
validation.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70749/
-----------------------------------------------------------
(Updated June 21, 2019, 3:32 p.m.)
Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
Bugs: MESOS-9809
https://issues.apache.org/jira/browse/MESOS-9809
Repository: mesos
Description (updated)
-------
This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the
As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.
See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.
Diffs (updated)
-----
3rdparty/libprocess/include/process/ssl/flags.hpp f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a
3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1
3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 6ef5a86566af3439cfe0b06ab3576076623f7be0
3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 29a1bf71c1df9d80370455a6269ecea0ec4193b0
Diff: https://reviews.apache.org/r/70749/diff/5/
Changes: https://reviews.apache.org/r/70749/diff/4-5/
Testing
-------
Todo!
Thanks,
Benno Evers