Re: Review Request 70749: WIP: Use openssl hostname validation.

[Benno Evers]

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

(Updated May 31, 2019, 3:47 p.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

WIP: Use openssl hostname validation.


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.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 


Diff: https://reviews.apache.org/r/70749/diff/1/


Testing
---


Thanks,

Benno Evers

Re: Review Request 70748: Disallow verification of empty TLS server certificates.

[Alexander Rukletsov]

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



The code looks good to me. Thanks!

As part of this change, either here or in separate reviews, let us:
- call out the modification in the changelog;
- mention taken approach in MESOS-9791 and explain why (based on the discussion 
we had with Jan-Philip Gehrcke);
- update SSL flags description, in particular calling out that `REQUIRE_CERT` 
is applicable for client mode only;
- update SSL documentation, in particular introduce suggested configuration and 
clarify the behaviour in client and server modes based on set arguments (based 
on the table you showed me offline);
- send a message to the user list regarding the use of anonymous ciphers


3rdparty/libprocess/src/openssl.cpp
Lines 742-743 (patched)


blank comment line please



3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 74-75 (patched)


ouch?



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 287-290 (patched)


Can we also test that a client must *not* present a cert? I believe the 
answer is _no_, but a TODO or a NOTE could be helpful.



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 290 (patched)


s/VerifyAnonymousCipher/NoAnonymousCipherIfVerify/



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 296 (patched)


Let's mention that `ADH-AES256-SHA` is an anonymous cipher and maybe even 
link https://www.openssl.org/docs/man1.0.2/man1/ciphers.html ?


- Alexander Rukletsov


On May 31, 2019, 2:34 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
> 
> (Updated May 31, 2019, 2:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-9791
> https://issues.apache.org/jira/browse/MESOS-9791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When in SSL client mode and `LIBPROCESS_SSL_VERIFY_CERT=true` has
> been set, enforce that the server actually presents a certificate
> that can be verified.
> 
> Note that in most cases, the TLS stack would rejected the connection
> before the code ever reaches `openssl::verify()`, since the TLS
> specification says that a server MUST always send a certificate
> unless an anonymous cipher is used.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 
> 
> 
> Diff: https://reviews.apache.org/r/70748/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benno Evers]

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


Fix it, then Ship it!




This looks pretty sweet, thanks for going the extra mile and improving our unit 
test suite!


src/tests/master/mock_master_api_subscriber.cpp
Lines 80 (patched)


Nit: No `.` at the end of log messages.

(same for the messages below)


- Benno Evers


On May 29, 2019, 7:17 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated May 29, 2019, 7:17 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces a class with mock methods for subscribing to the
> events of master's V1 streaming API and setting expectations for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION 
>   src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70671/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70748: Disallow verification of empty TLS server certificates.

[Benno Evers]

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

(Updated May 31, 2019, 2:34 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, and Till 
Toenshoff.


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


Repository: mesos


Description
---

When in SSL client mode and `LIBPROCESS_SSL_VERIFY_CERT=true` has
been set, enforce that the server actually presents a certificate
that can be verified.

Note that in most cases, the TLS stack would rejected the connection
before the code ever reaches `openssl::verify()`, since the TLS
specification says that a server MUST always send a certificate
unless an anonymous cipher is used.


Diffs
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70748/diff/1/


Testing
---


Thanks,

Benno Evers

Re: Review Request 70748: Disallow verification of empty TLS server certificates.

[Benno Evers]

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

(Updated May 31, 2019, 2:06 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, and Till 
Toenshoff.


Repository: mesos


Description (updated)
---

When in SSL client mode and `LIBPROCESS_SSL_VERIFY_CERT=true` has
been set, enforce that the server actually presents a certificate
that can be verified.

Note that in most cases, the TLS stack would rejected the connection
before the code ever reaches `openssl::verify()`, since the TLS
specification says that a server MUST always send a certificate
unless an anonymous cipher is used.


Diffs
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70748/diff/1/


Testing
---


Thanks,

Benno Evers

Re: Review Request 70747: Updated some flag description for libprocess SSL flags.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On May 29, 2019, 1:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70747/
> ---
> 
> (Updated May 29, 2019, 1:50 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed `SSL` to `TLS` in some flag descriptions where the
> flag was referring to versions of the TLS protocol.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
> 
> 
> Diff: https://reviews.apache.org/r/70747/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70757: Added a privs isolator.

[Andrei Budnik]

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




src/slave/containerizer/mesos/launch.cpp
Lines 1098 (patched)


Can this condition ever be `false`, given the isolator assigns `true` in 
any case?

What is the semantics of `ContainerLaunchInfo::no_new_privileges` flag? How 
does it work in pair with the new isolator?



src/slave/containerizer/mesos/launch.cpp
Lines 1099 (patched)


`PR_SET_NO_NEW_PRIVS` requires Linux kernel 3.5 (according to `man 2 
prctl`).

Minimum supported kernel version by Mesos is 2.6.28 - see 
http://mesos.apache.org/documentation/latest/building/#system-requirements

There are at least 2 options to fix this problem:
1) bump the minimum kernel version to 3.5, if no one is against it on the 
dev/user list
2) remove `include ` and define `PR_SET_NO_NEW_PRIVS` in the 
`containerizer/mesos/launch.cpp`. See 
https://github.com/apache/mesos/blob/fa410f2fb8efb988590f4da2d4cfffbb2ce70637/src/slave/containerizer/mesos/isolators/linux/seccomp.cpp#L35-L41
 and 
https://github.com/seccomp/libseccomp/blob/78497a5d1da200ab0356e1189f5efb8724ad70a1/src/system.h#L94-L97


- Andrei Budnik


On May 31, 2019, 4:02 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated May 31, 2019, 4:02 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> b2e35cbf01caea6c1e4f45c7b7d833bc7f065099 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 043244841a73fa3f5f7119bc38f6d3a04be8990b 
>   src/slave/containerizer/mesos/isolators/linux/privs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/privs.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> 0c482f46a97063133edfe29ae3c6a2721d29f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 70741: Adopted container file operations for secrets volumes.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70741]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 29, 2019, 4:29 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> ---
> 
> (Updated May 29, 2019, 4:29 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> b2e35cbf01caea6c1e4f45c7b7d833bc7f065099 
>   src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc 
>   src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 7a9bb82b23b35728408fb37bac53d79883c0a19f 
>   src/slave/containerizer/mesos/launch.cpp 
> 0c482f46a97063133edfe29ae3c6a2721d29f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/70741/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>