----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46370/#review132094 -----------------------------------------------------------
See my detailed comments. I think we're introducing too many classes in this library code, making it very confusing. src/linux/capabilities.hpp (lines 37 - 39) <https://reviews.apache.org/r/46370/#comment196187> I would suggest that we put all capability related stuff in mesos::internal::capability namespace. Since we'll be converting from enum to int constantly, i would prefer we use `int` and `constexpr` for those constants. ``` constexpr int CHOWN = 0; constexpr int DAC_override = 1; ... ``` The callsites read well: ``` capability::SYS_ADMIN ``` src/linux/capabilities.hpp (line 88) <https://reviews.apache.org/r/46370/#comment196238> what about BOUNDING? Do we need to get that as well? Looking at the go capability library that docker uses: https://github.com/syndtr/gocapability/blob/master/capability/enum.go#L31 src/linux/capabilities.hpp (line 194) <https://reviews.apache.org/r/46370/#comment196240> When I review the code, I found very confusing. We have `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of `Capabilities`, it's not straightfoward to extend it to support file capabilities. If possible, I'd like the library here that's easily extensible to support file capabilities. In fact, I like the abstraction in the go library https://github.com/syndtr/gocapability/blob/master/capability/capability.go So, basically, `Capabilities` will become the struct that contains all capabilities (permitted, inheritted, etc) if applied. and it has methods defined allowing callers to modify any of them. Finally, there's an `Apply` method which can be used to apply the change. The methods will be virtual so that in the future, we can use that to support file capabilities as well: ``` typedef int Capability constexpr int CHOWN = 0; ... enum Type { EFFECTIVE, PERMITTED, INHERITABLE, BOUNDING }; class Capabilities { public: static Try<Owned<Capabilities>> fromPid(const Option<pid_t>& pid = None()); static Try<Owned<Capabilities>> fromFile(const string& path); // TODO. virtual bool test(Type type, Capability cap); virtual void set(Type type, const hashset<Capability>& caps); virtual void unset(Type type, const hashset<Capability>& caps); virtual void fill(Type type); virtual void clear(Type type); virtual Try<Nothing> apply(); }; // Returns the set of capabilities specified in the protobuf. hashset<Capability> parse(const CapabilityInfo& info); ``` In the cpp file, you can create a `class ProcessCapabilities : public Capabilities` that implements the above virtual methods. For version and lastCapability, i think it can be a global variable in the cpp file which will be set the first time `version` is called. Any subsequent calling of `version` will use the cached value: ``` Try<uint32_t> version(); Try<Capability> lastCapability(); Try<Owned<Capabilities>> ProcessCapabilities::create(const Option<pid_t>& pid = None()) { // you can save version and lastCap in ProcessCapabilities. } ``` src/linux/capabilities.hpp (lines 267 - 282) <https://reviews.apache.org/r/46370/#comment196192> Can you do that in a separate patch? src/linux/capabilities.cpp (line 341) <https://reviews.apache.org/r/46370/#comment196193> I think 'version' is a 32 bit unsigned integer. Can you use uint32_t instead? src/linux/capabilities.cpp (line 581) <https://reviews.apache.org/r/46370/#comment196273> Why we allow caller to specify a cap as a string? Are you worried about the parsing? Can we just allow operator to specify a JSON of type `CapabilityInfo`? - Jie Yu On May 6, 2016, 5:03 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46370/ > ----------------------------------------------------------- > > (Updated May 6, 2016, 5:03 p.m.) > > > Review request for mesos, Jie Yu and Kevin Klues. > > > Bugs: MESOS-5051 > https://issues.apache.org/jira/browse/MESOS-5051 > > > Repository: mesos > > > Description > ------- > > This change introduces basic API for linux capabilities. This is not a > comprehensive API but is strictly limited to the need for securing Mesos > containers using linux capabilities. > > > Diffs > ----- > > src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 > src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 > src/linux/capabilities.hpp PRE-CREATION > src/linux/capabilities.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46370/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jojy Varghese > >
