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 2017/05/11 16:44:50 UTC
Review Request 59185: Add ambient capability support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description
-------
In the absence of ambient capabilities, capabilities in the
effective set do not survive across execve(2). This means
that tasks attempting to make use of the LinuxInfo capability
support also need to ensure that file capabilities are set on
the file that is ultimately executed. Supporting ambient
capabilities allows the effective capabilities to survive
execve(2), so it is now possible to launch a task with limited
privilege elevations.
Diffs
-----
src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
src/slave/containerizer/mesos/launch.cpp 2835beff9dbfa7f2a1cac306a58e2b1d66c14342
src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
Diff: https://reviews.apache.org/r/59185/diff/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59185: Add ambient capability support.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177577
-----------------------------------------------------------
Bad patch!
Reviews applied: [59185, 59552, 59551, 59550, 59549, 59548, 59547]
Failed command: python support/apply-reviews.py -n -r 59547
Error:
error: patch failed: src/launcher/executor.cpp:631
error: src/launcher/executor.cpp: patch does not apply
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/85/console
- Mesos Reviewbot Windows
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/5/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177579
-----------------------------------------------------------
Please fix the review board dependency. Looks like there are still patches following this patch, but I don't see "blockes" field. That'll fail the reviewbot.
- Jie Yu
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/5/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/
-----------------------------------------------------------
(Updated June 11, 2017, 6:34 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-7477
https://issues.apache.org/jira/browse/MESOS-7477
Repository: mesos
Description
-------
Add support for the ambient capability so that we can make
effective capabilities survive across execve(2).
Diffs (updated)
-----
src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
Diff: https://reviews.apache.org/r/59185/diff/6/
Changes: https://reviews.apache.org/r/59185/diff/5-6/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59185: Add ambient capability support.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177557
-----------------------------------------------------------
Bad patch!
Reviews applied: [59185, 59552, 59551, 59550, 59549, 59548, 59547]
Failed command: python support/apply-reviews.py -n -r 59547
Error:
error: patch failed: src/launcher/executor.cpp:631
error: src/launcher/executor.cpp: patch does not apply
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/85/console
- Mesos Reviewbot Windows
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
> On June 11, 2017, 5:20 p.m., Jie Yu wrote:
> > src/linux/capabilities.cpp
> > Lines 256-258 (patched)
> > <https://reviews.apache.org/r/59185/diff/4/?file=1747638#file1747638line256>
> >
> > I would move the implementation of `_ambientCapabilitiesSupported` to this constructor.
>
> James Peach wrote:
> It is done this way so that `Capabilities::ambientCapabilitiesSupported` can be a const data member.
>
> Jie Yu wrote:
> ah, ic, can you do
> ```
> Capabilities::Capabilities(int _lastCap)
> : ambientCapabilitiesSupported([]() {
> ...
> }()),
> lastCap(_lastCap) {}
> ```
That makes my eyes bleed a bit, but I can do that :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177568
-----------------------------------------------------------
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by Jie Yu <yu...@gmail.com>.
> On June 11, 2017, 5:20 p.m., Jie Yu wrote:
> > src/linux/capabilities.cpp
> > Lines 256-258 (patched)
> > <https://reviews.apache.org/r/59185/diff/4/?file=1747638#file1747638line256>
> >
> > I would move the implementation of `_ambientCapabilitiesSupported` to this constructor.
>
> James Peach wrote:
> It is done this way so that `Capabilities::ambientCapabilitiesSupported` can be a const data member.
ah, ic, can you do
```
Capabilities::Capabilities(int _lastCap)
: ambientCapabilitiesSupported([]() {
...
}()),
lastCap(_lastCap) {}
```
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177568
-----------------------------------------------------------
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
> On June 11, 2017, 5:20 p.m., Jie Yu wrote:
> > src/linux/capabilities.cpp
> > Lines 256-258 (patched)
> > <https://reviews.apache.org/r/59185/diff/4/?file=1747638#file1747638line256>
> >
> > I would move the implementation of `_ambientCapabilitiesSupported` to this constructor.
It is done this way so that `Capabilities::ambientCapabilitiesSupported` can be a const data member.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177568
-----------------------------------------------------------
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/#review177568
-----------------------------------------------------------
Fix it, then Ship it!
Nice tests!
src/linux/capabilities.cpp
Lines 256-258 (patched)
<https://reviews.apache.org/r/59185/#comment251258>
I would move the implementation of `_ambientCapabilitiesSupported` to this constructor.
src/linux/capabilities.cpp
Lines 254-256 (original), 322-325 (patched)
<https://reviews.apache.org/r/59185/#comment251257>
I would inline the implementation here. We usually do not factor out a helper if it is only used in one place.
If you do want to facator out a helper, inline lambda is better than a static method.
src/tests/containerizer/capabilities_tests.cpp
Lines 201-202 (patched)
<https://reviews.apache.org/r/59185/#comment251259>
Style nits: This should be
```
wanted.set(
capabilities::INHERITABLE,
{capabilities::SETPCAP, capabilities::CHOWN});
```
src/tests/containerizer/capabilities_tests.cpp
Lines 203-204 (patched)
<https://reviews.apache.org/r/59185/#comment251260>
Ditto.
src/tests/containerizer/capabilities_tests.cpp
Lines 207-208 (patched)
<https://reviews.apache.org/r/59185/#comment251261>
Ditto.
- Jie Yu
On June 5, 2017, 4:50 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59185/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:50 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7477
> https://issues.apache.org/jira/browse/MESOS-7477
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for the ambient capability so that we can make
> effective capabilities survive across execve(2).
>
>
> Diffs
> -----
>
> src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
> src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
> src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
>
>
> Diff: https://reviews.apache.org/r/59185/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/
-----------------------------------------------------------
(Updated June 5, 2017, 4:50 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-7477
https://issues.apache.org/jira/browse/MESOS-7477
Repository: mesos
Description
-------
Add support for the ambient capability so that we can make
effective capabilities survive across execve(2).
Diffs (updated)
-----
src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
Diff: https://reviews.apache.org/r/59185/diff/3/
Changes: https://reviews.apache.org/r/59185/diff/2-3/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/
-----------------------------------------------------------
(Updated May 24, 2017, 11:46 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-7477
https://issues.apache.org/jira/browse/MESOS-7477
Repository: mesos
Description (updated)
-------
Add support for the ambient capability so that we can make
effective capabilities survive across execve(2).
Diffs (updated)
-----
src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
Diff: https://reviews.apache.org/r/59185/diff/2/
Changes: https://reviews.apache.org/r/59185/diff/1-2/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59185: Add ambient capability support.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59185/
-----------------------------------------------------------
(Updated May 11, 2017, 4:45 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-7477
https://issues.apache.org/jira/browse/MESOS-7477
Repository: mesos
Description
-------
In the absence of ambient capabilities, capabilities in the
effective set do not survive across execve(2). This means
that tasks attempting to make use of the LinuxInfo capability
support also need to ensure that file capabilities are set on
the file that is ultimately executed. Supporting ambient
capabilities allows the effective capabilities to survive
execve(2), so it is now possible to launch a task with limited
privilege elevations.
Diffs
-----
src/linux/capabilities.hpp 5fa3799948f8ac4bcaeaa89f91d7d090e426c5a6
src/linux/capabilities.cpp 7aa8c352def644468e8c9041a2fe4319f313b09b
src/slave/containerizer/mesos/launch.cpp 2835beff9dbfa7f2a1cac306a58e2b1d66c14342
src/tests/containerizer/capabilities_tests.cpp 15a85cab87c28402eeb2bfbc751c8c77bf4c14f5
Diff: https://reviews.apache.org/r/59185/diff/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach