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