----------------------------------------------------------- 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 > >
