You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2019/05/19 23:22:35 UTC
Review Request 70678: Add containerizer support for masking paths.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-9771
https://issues.apache.org/jira/browse/MESOS-9771
Repository: mesos
Description
-------
Add support to the `filesystem/linux` isolator for masking container
paths. Add a set of standard default paths to be masked, as derived
from commonly used container runtimes. These paths either expose
information about other system processes, or capabilities that
should not be exposed to untristed containers.
We don't mask if the container is privileged, which is defined
as sharing the host's PID namespace. However, there is no way to
tell whether a nested container will ultimaty be privileged by
this definition.
Diffs
-----
include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 7b50258ef5480c1ea3f0016aace3b838395becfd
src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
Diff: https://reviews.apache.org/r/70678/diff/1/
Testing
-------
sudo make check (Fedora 30)
Thanks,
James Peach
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by James Peach <jp...@apache.org>.
> On May 21, 2019, 10:16 p.m., Jason Lai wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 816 (patched)
> > <https://reviews.apache.org/r/70678/diff/3/?file=2145849#file2145849line816>
> >
> > Nit: I feel we should consider making the masked paths an instance variable of the isolator class and initializing it with `ROOTFS_MASKED_PATHS` instead, in the purpose of avoid hard coding.
I'd rather leave this as it is for now. We can make this change if we ever need to make the set configurable.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215433
-----------------------------------------------------------
On May 22, 2019, 8:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 8:25 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215433
-----------------------------------------------------------
Fix it, then Ship it!
Good stuff! LGTM overall with some nits.
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 500 (patched)
<https://reviews.apache.org/r/70678/#comment302131>
Nit: `return false` first, so we won't need to nest the logic inside the previous `if` statement.
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 816 (patched)
<https://reviews.apache.org/r/70678/#comment302130>
Nit: I feel we should consider making the masked paths an instance variable of the isolator class and initializing it with `ROOTFS_MASKED_PATHS` instead, in the purpose of avoid hard coding.
- Jason Lai
On May 20, 2019, 1:41 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 20, 2019, 1:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 7b50258ef5480c1ea3f0016aace3b838395becfd
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215491
-----------------------------------------------------------
Ship it!
Ship It!
- Jason Lai
On May 22, 2019, 8:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 8:25 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215477
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70678]
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 22, 2019, 8:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 8:25 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215489
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On May 22, 2019, 1:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 1:25 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/
-----------------------------------------------------------
(Updated May 22, 2019, 8:25 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-9771
https://issues.apache.org/jira/browse/MESOS-9771
Repository: mesos
Description
-------
Add support to the `filesystem/linux` isolator for masking container
paths. Add a set of standard default paths to be masked, as derived
from commonly used container runtimes. These paths either expose
information about other system processes, or capabilities that
should not be exposed to untrusted containers.
We don't mask if the container is privileged, which is defined
as sharing the host's PID namespace. For nested containers, we
verify that the PID namespace is shared from the host all the way
up the tree.
Diffs (updated)
-----
include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7
src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
Diff: https://reviews.apache.org/r/70678/diff/4/
Changes: https://reviews.apache.org/r/70678/diff/3-4/
Testing
-------
sudo make check (Fedora 30)
Thanks,
James Peach
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215351
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70678]
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 20, 2019, 1:41 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 20, 2019, 1:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 7b50258ef5480c1ea3f0016aace3b838395becfd
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/
-----------------------------------------------------------
(Updated May 20, 2019, 1:41 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-9771
https://issues.apache.org/jira/browse/MESOS-9771
Repository: mesos
Description (updated)
-------
Add support to the `filesystem/linux` isolator for masking container
paths. Add a set of standard default paths to be masked, as derived
from commonly used container runtimes. These paths either expose
information about other system processes, or capabilities that
should not be exposed to untristed containers.
We don't mask if the container is privileged, which is defined as
sharing the host's PID namespace. For nested containers, we verify
that the PID namespace is shared from the host annd the way up
the tree.
Diffs (updated)
-----
include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 7b50258ef5480c1ea3f0016aace3b838395becfd
src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
Diff: https://reviews.apache.org/r/70678/diff/2/
Changes: https://reviews.apache.org/r/70678/diff/1-2/
Testing
-------
sudo make check (Fedora 30)
Thanks,
James Peach
Re: Review Request 70678: Add containerizer support for masking paths.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70678/#review215350
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70678]
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 19, 2019, 11:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> -----------------------------------------------------------
>
> (Updated May 19, 2019, 11:22 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untristed containers.
>
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. However, there is no way to
> tell whether a nested container will ultimaty be privileged by
> this definition.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 7b50258ef5480c1ea3f0016aace3b838395becfd
> src/slave/containerizer/mesos/launch.cpp 88b97a572916defbe65692036be77395053eb8e8
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 60e9ae5970a0a45314d0b3569556bef36d350d2b
> src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe
>
>
> Diff: https://reviews.apache.org/r/70678/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>