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