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/24 23:46:01 UTC
Review Request 59552: Add support for explicitly setting bounding
capabilities.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description
-------
The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.
Diffs
-----
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
Diff: https://reviews.apache.org/r/59552/diff/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
> On June 11, 2017, 6:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 642-643 (patched)
> > <https://reviews.apache.org/r/59552/diff/3/?file=1747441#file1747441line642>
> >
> > Let's make sure inheritable is always a subset of bounding. This prevents a container from getting permitted capabilities beyond its bounding set by crafting an executable with proper `F(inheritable)` set.
Right below this, we force inheritable to be the same as bounding to address this issue. I think this was a stale diff :-/
> On June 11, 2017, 6:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 91-92 (patched)
> > <https://reviews.apache.org/r/59552/diff/4/?file=1747634#file1747634line93>
> >
> > I wouldn't do that here. I think the goal is to allow framework to specify bounding as well in the future. I would set bounding below.
> >
> > Currently, framework cannot set bounding. We'll always use the operator specified bounding if set. If not set, set bounding to effective if set.
> >
> > ```
> > if (containerConfig.has_container_info() ...) {
> > effective = ...;
> > }
> >
> > if (effective.isNone()) {
> > effective = flags.allowed_capabilities;
> > }
> >
> > // NOTE: Currently, we do not allow framework to set
> > // bounding capabilities separately. Therefore, it'll
> > // always be what the operator has specified.
> > bounding = flags.bounding_capabilities;
> >
> > // NOTE: This is for backwards compatibility.
> > if (effective.isSome() && bounding.isNone()) {
> > bounding = effective;
> > }
> > ```
This is done because the `effective` set it guaranteed to be equal or smaller than the `bounding` set. If we use the operator `bounding_capabilities` here, then the task can gain more privilege than the framework specifies. If the framework specifies a set of capabilities, it should get exactly that and no more.
> On June 11, 2017, 6:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Line 93 (original), 112 (patched)
> > <https://reviews.apache.org/r/59552/diff/4/?file=1747634#file1747634line114>
> >
> > I would use 'bounding' here instead of `flags.bounding_capabilites`. in the future, we will allow framework to specify bounding set.
You can't use `bounding` here because `bounding` might be set from the framework's capabilities. We need to ensure that whatever the framework specifies is within the limits set by the operator. That will still be true when the framework gets to specify the bounding set.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review177551
-----------------------------------------------------------
On June 10, 2017, 9:43 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 10, 2017, 9:43 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/5/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review177551
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 159 (patched)
<https://reviews.apache.org/r/59552/#comment251231>
This variable is not necessary if you follow the suggest i have in the above comment.
src/slave/containerizer/mesos/launch.cpp
Lines 642-643 (patched)
<https://reviews.apache.org/r/59552/#comment251216>
Let's make sure inheritable is always a subset of bounding. This prevents a container from getting permitted capabilities beyond its bounding set by crafting an executable with proper `F(inheritable)` set.
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 91-92 (patched)
<https://reviews.apache.org/r/59552/#comment251266>
I wouldn't do that here. I think the goal is to allow framework to specify bounding as well in the future. I would set bounding below.
Currently, framework cannot set bounding. We'll always use the operator specified bounding if set. If not set, set bounding to effective if set.
```
if (containerConfig.has_container_info() ...) {
effective = ...;
}
if (effective.isNone()) {
effective = flags.allowed_capabilities;
}
// NOTE: Currently, we do not allow framework to set
// bounding capabilities separately. Therefore, it'll
// always be what the operator has specified.
bounding = flags.bounding_capabilities;
// NOTE: This is for backwards compatibility.
if (effective.isSome() && bounding.isNone()) {
bounding = effective;
}
```
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Line 93 (original), 112 (patched)
<https://reviews.apache.org/r/59552/#comment251265>
I would use 'bounding' here instead of `flags.bounding_capabilites`. in the future, we will allow framework to specify bounding set.
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 131 (patched)
<https://reviews.apache.org/r/59552/#comment251268>
CHECK_SOME
src/slave/containerizer/mesos/launch.cpp
Lines 642-643 (patched)
<https://reviews.apache.org/r/59552/#comment251264>
Style nits. I saw we have the following in the above code
```
set<Capability> target = capabilities::convert(
launchInfo.effective_capabilities());
```
Let's make sure the style are consistent. either your way, or the previous way, but not mixed.
src/slave/containerizer/mesos/launch.cpp
Lines 653-654 (patched)
<https://reviews.apache.org/r/59552/#comment251263>
Style nit:
```
capabilities->set(
capabilities::INHERITABLE,
capabilities->get(capabilities::BOUNDING));
```
- Jie Yu
On June 10, 2017, 9:43 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 10, 2017, 9:43 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/
-----------------------------------------------------------
(Updated June 16, 2017, 4:39 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebased and addressed review feedback.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description (updated)
-------
The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.
Removed 2 capabilities isolator test cases. These test cases depended on
the framework-specified effective capabilities also setting the bounding
set. Now that the operator flag always determines the bounding set,
these test cases are no longer valid.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
Diff: https://reviews.apache.org/r/59552/diff/7/
Changes: https://reviews.apache.org/r/59552/diff/6-7/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/
-----------------------------------------------------------
(Updated June 10, 2017, 9:43 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description
-------
The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
Diff: https://reviews.apache.org/r/59552/diff/4/
Changes: https://reviews.apache.org/r/59552/diff/3-4/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/
-----------------------------------------------------------
(Updated June 9, 2017, 11:09 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebase and partially address review feedback.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description
-------
The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
Diff: https://reviews.apache.org/r/59552/diff/3/
Changes: https://reviews.apache.org/r/59552/diff/2-3/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 99-103 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741704#file1741704line101>
> >
> > In fact, i think the semantics should be:
> >
> > 1) treat framework specificied capabilities as effective capabilities.
> > 2) in the future, allow frameworks to set bounding capabilities
> > 3) if effective capabilities is set and bounding capabilities is not set, set bounding set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
> > 4) treat operator flags as the default values if the framework does not specify them (for both bounding and effective capabilities).
> > 5) if both effective and bounding capabilities are set, make sure the effective is a subset of bounding
> > 6) if effective capabilities is not set, but bounding capabilities is set, set effective capabilities to be the same as bounding capabilities
> >
> > Given that, I'd adjust the logic of this function to the following:
> >
> > ```
> > Option<CapabilityInfo> effective;
> > Option<CapabilityInfo> bounding;
> >
> > if (containerConfig.has_container_info() &&
> > containerConfig.container_info().has_linux_info() &&
> > containerConfig.container_info().linux_info().has_capability_info()) {
> > effective = containerConfig.container_info().linux_info().capability_info();
> > }
> >
> > // Framework can override the effective capabilities.
> > if (effective.isNone() && flags.effective_capabilities.isSome()) {
> > effective = flags.effective_capabilities.get();
> > }
> >
> > if (flags.bounding_capabilities.isSome()) {
> > bounding = flags.bounding_capabilities.get();
> > }
> >
> > if (effective.isSome() && bounding.isSome()) {
> > // Validate that effective is a subset of bounding.
> > }
> >
> > if (effective.isSome() && bounding.isNone()) {
> > bounding = effective.get();
> > }
> >
> > if (effective.isNone() && bounding.isSome()) {
> > effective = bounding.get();
> > }
> >
> > ...
> > ```
> >
> > The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.
>
> James Peach wrote:
> Thinking some more about this one :)
>
> Jie Yu wrote:
> I'd still use the logic above with some tweeks. Basically, calculate effective and bounding sets first. Instead of splitting them into two separate funcitons (one in prepare, one in mergeCapabilities). The code shown above is much easier to follow.
>
> James Peach wrote:
> Yes, this change made the code much clearer. I don't think we should do (6), since when ambient privileges are implemented it means the task will actually hold the capabilities in the bounding set, which is the opposite of what you want.
Yes. let's do 1-5.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review176530
-----------------------------------------------------------
On June 10, 2017, 9:43 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 10, 2017, 9:43 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 99-103 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741704#file1741704line101>
> >
> > In fact, i think the semantics should be:
> >
> > 1) treat framework specificied capabilities as effective capabilities.
> > 2) in the future, allow frameworks to set bounding capabilities
> > 3) if effective capabilities is set and bounding capabilities is not set, set bounding set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
> > 4) treat operator flags as the default values if the framework does not specify them (for both bounding and effective capabilities).
> > 5) if both effective and bounding capabilities are set, make sure the effective is a subset of bounding
> > 6) if effective capabilities is not set, but bounding capabilities is set, set effective capabilities to be the same as bounding capabilities
> >
> > Given that, I'd adjust the logic of this function to the following:
> >
> > ```
> > Option<CapabilityInfo> effective;
> > Option<CapabilityInfo> bounding;
> >
> > if (containerConfig.has_container_info() &&
> > containerConfig.container_info().has_linux_info() &&
> > containerConfig.container_info().linux_info().has_capability_info()) {
> > effective = containerConfig.container_info().linux_info().capability_info();
> > }
> >
> > // Framework can override the effective capabilities.
> > if (effective.isNone() && flags.effective_capabilities.isSome()) {
> > effective = flags.effective_capabilities.get();
> > }
> >
> > if (flags.bounding_capabilities.isSome()) {
> > bounding = flags.bounding_capabilities.get();
> > }
> >
> > if (effective.isSome() && bounding.isSome()) {
> > // Validate that effective is a subset of bounding.
> > }
> >
> > if (effective.isSome() && bounding.isNone()) {
> > bounding = effective.get();
> > }
> >
> > if (effective.isNone() && bounding.isSome()) {
> > effective = bounding.get();
> > }
> >
> > ...
> > ```
> >
> > The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.
Thinking some more about this one :)
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Line 456 (original), 456-457 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741705#file1741705line456>
> >
> > We probably should have a CHECK (or comment) here. both of them are either set, or either not set. Maybe add a comment in the flags as well.
The effective and bounding capabilities are currently allowed to vary (see below).
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 627 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741705#file1741705line627>
> >
> > Since two flags are either both set or both not set. I would simply check both are isSome() here.
They are actually allowed to vary. If the neither the framework nor the operator specified effective capabilities, then `launchInfo.has_effective_capabilities()` would be false but `launchInfo.has_bounding_capabilities()` could still be true.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review176530
-----------------------------------------------------------
On June 5, 2017, 4:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:36 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 99-103 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741704#file1741704line101>
> >
> > In fact, i think the semantics should be:
> >
> > 1) treat framework specificied capabilities as effective capabilities.
> > 2) in the future, allow frameworks to set bounding capabilities
> > 3) if effective capabilities is set and bounding capabilities is not set, set bounding set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
> > 4) treat operator flags as the default values if the framework does not specify them (for both bounding and effective capabilities).
> > 5) if both effective and bounding capabilities are set, make sure the effective is a subset of bounding
> > 6) if effective capabilities is not set, but bounding capabilities is set, set effective capabilities to be the same as bounding capabilities
> >
> > Given that, I'd adjust the logic of this function to the following:
> >
> > ```
> > Option<CapabilityInfo> effective;
> > Option<CapabilityInfo> bounding;
> >
> > if (containerConfig.has_container_info() &&
> > containerConfig.container_info().has_linux_info() &&
> > containerConfig.container_info().linux_info().has_capability_info()) {
> > effective = containerConfig.container_info().linux_info().capability_info();
> > }
> >
> > // Framework can override the effective capabilities.
> > if (effective.isNone() && flags.effective_capabilities.isSome()) {
> > effective = flags.effective_capabilities.get();
> > }
> >
> > if (flags.bounding_capabilities.isSome()) {
> > bounding = flags.bounding_capabilities.get();
> > }
> >
> > if (effective.isSome() && bounding.isSome()) {
> > // Validate that effective is a subset of bounding.
> > }
> >
> > if (effective.isSome() && bounding.isNone()) {
> > bounding = effective.get();
> > }
> >
> > if (effective.isNone() && bounding.isSome()) {
> > effective = bounding.get();
> > }
> >
> > ...
> > ```
> >
> > The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.
>
> James Peach wrote:
> Thinking some more about this one :)
>
> Jie Yu wrote:
> I'd still use the logic above with some tweeks. Basically, calculate effective and bounding sets first. Instead of splitting them into two separate funcitons (one in prepare, one in mergeCapabilities). The code shown above is much easier to follow.
Yes, this change made the code much clearer. I don't think we should do (6), since when ambient privileges are implemented it means the task will actually hold the capabilities in the bounding set, which is the opposite of what you want.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review176530
-----------------------------------------------------------
On June 9, 2017, 11:09 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:09 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 99-103 (patched)
> > <https://reviews.apache.org/r/59552/diff/2/?file=1741704#file1741704line101>
> >
> > In fact, i think the semantics should be:
> >
> > 1) treat framework specificied capabilities as effective capabilities.
> > 2) in the future, allow frameworks to set bounding capabilities
> > 3) if effective capabilities is set and bounding capabilities is not set, set bounding set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
> > 4) treat operator flags as the default values if the framework does not specify them (for both bounding and effective capabilities).
> > 5) if both effective and bounding capabilities are set, make sure the effective is a subset of bounding
> > 6) if effective capabilities is not set, but bounding capabilities is set, set effective capabilities to be the same as bounding capabilities
> >
> > Given that, I'd adjust the logic of this function to the following:
> >
> > ```
> > Option<CapabilityInfo> effective;
> > Option<CapabilityInfo> bounding;
> >
> > if (containerConfig.has_container_info() &&
> > containerConfig.container_info().has_linux_info() &&
> > containerConfig.container_info().linux_info().has_capability_info()) {
> > effective = containerConfig.container_info().linux_info().capability_info();
> > }
> >
> > // Framework can override the effective capabilities.
> > if (effective.isNone() && flags.effective_capabilities.isSome()) {
> > effective = flags.effective_capabilities.get();
> > }
> >
> > if (flags.bounding_capabilities.isSome()) {
> > bounding = flags.bounding_capabilities.get();
> > }
> >
> > if (effective.isSome() && bounding.isSome()) {
> > // Validate that effective is a subset of bounding.
> > }
> >
> > if (effective.isSome() && bounding.isNone()) {
> > bounding = effective.get();
> > }
> >
> > if (effective.isNone() && bounding.isSome()) {
> > effective = bounding.get();
> > }
> >
> > ...
> > ```
> >
> > The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.
>
> James Peach wrote:
> Thinking some more about this one :)
I'd still use the logic above with some tweeks. Basically, calculate effective and bounding sets first. Instead of splitting them into two separate funcitons (one in prepare, one in mergeCapabilities). The code shown above is much easier to follow.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review176530
-----------------------------------------------------------
On June 9, 2017, 11:09 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:09 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/#review176530
-----------------------------------------------------------
src/slave/containerizer/mesos/launch.cpp
Line 454 (original), 454 (patched)
<https://reviews.apache.org/r/59552/#comment249903>
I suggest we use Option here. `Result` here is super wierd.
src/slave/containerizer/mesos/launch.cpp
Line 457 (original), 458 (patched)
<https://reviews.apache.org/r/59552/#comment249905>
```
Try<Capabilities> _capabilitiesManager = Capabilities::create();
if (_capabilitiesManager.isError()) {
...
}
capabilitiesManager = _capabilitiesManager.get();
```
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 99-103 (patched)
<https://reviews.apache.org/r/59552/#comment251128>
In fact, i think the semantics should be:
1) treat framework specificied capabilities as effective capabilities.
2) in the future, allow frameworks to set bounding capabilities
3) if effective capabilities is set and bounding capabilities is not set, set bounding set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
4) treat operator flags as the default values if the framework does not specify them (for both bounding and effective capabilities).
5) if both effective and bounding capabilities are set, make sure the effective is a subset of bounding
6) if effective capabilities is not set, but bounding capabilities is set, set effective capabilities to be the same as bounding capabilities
Given that, I'd adjust the logic of this function to the following:
```
Option<CapabilityInfo> effective;
Option<CapabilityInfo> bounding;
if (containerConfig.has_container_info() &&
containerConfig.container_info().has_linux_info() &&
containerConfig.container_info().linux_info().has_capability_info()) {
effective = containerConfig.container_info().linux_info().capability_info();
}
// Framework can override the effective capabilities.
if (effective.isNone() && flags.effective_capabilities.isSome()) {
effective = flags.effective_capabilities.get();
}
if (flags.bounding_capabilities.isSome()) {
bounding = flags.bounding_capabilities.get();
}
if (effective.isSome() && bounding.isSome()) {
// Validate that effective is a subset of bounding.
}
if (effective.isSome() && bounding.isNone()) {
bounding = effective.get();
}
if (effective.isNone() && bounding.isSome()) {
effective = bounding.get();
}
...
```
The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.
src/slave/containerizer/mesos/launch.cpp
Line 456 (original), 456-457 (patched)
<https://reviews.apache.org/r/59552/#comment251134>
We probably should have a CHECK (or comment) here. both of them are either set, or either not set. Maybe add a comment in the flags as well.
src/slave/containerizer/mesos/launch.cpp
Lines 627 (patched)
<https://reviews.apache.org/r/59552/#comment251140>
Since two flags are either both set or both not set. I would simply check both are isSome() here.
- Jie Yu
On June 5, 2017, 4:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 4:36 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
> src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
> src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
>
>
> Diff: https://reviews.apache.org/r/59552/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59552: Add support for explicitly setting bounding
capabilities.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59552/
-----------------------------------------------------------
(Updated June 5, 2017, 4:36 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-7476
https://issues.apache.org/jira/browse/MESOS-7476
Repository: mesos
Description
-------
The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da
src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce
src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1
Diff: https://reviews.apache.org/r/59552/diff/2/
Changes: https://reviews.apache.org/r/59552/diff/1-2/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach