> On May 7, 2016, 7:14 p.m., Jie Yu wrote: > > src/linux/capabilities.hpp, line 194 > > <https://reviews.apache.org/r/46370/diff/7/?file=1374923#file1374923line194> > > > > 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. > > } > > ``` > > Jojy Varghese wrote: > Thanks for the review Jie. > > The idea behind separate classes is to keep the functionality cohesive > which I think is an important property of any API. I believe that `test`, > `fill`, `clear` etc is not part of the `capabilities` domain but they change > state of `flags`. `Flags` are first class citizens of the capabilities > domain. Also, ideally the `capabilities` API should only have `set` and > `get`, i had to add `drop` and others to satisfy some of our other > requirements. I think if i pull them out into functions(not part of the > class), the `capabilities` class will have only `set` and `get` of flags. > > Also, `set` is an array of `capabilities`. In linux, libcap is supposed > to be the standard portable API and I tried to follow that API more that the > `go` API. In libcap, there are functions like `cap_set_flag` to set a > particular capabilility (CHOWN) in a set > (`http://linux.die.net/man/3/cap_set_flag`). I have tried to put those as > part of the `CapabiityFlag` and `CapabilitySet` classes. > > Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as > first class citizens of the API. For example libcap uses `cap_value_t` as the > type for the capabilities. I added `enum Capability` as the type. > > Jie Yu wrote: > I understand that your interface is modeled after libcap. I looked at > both and found that libcap interfaces are a bit confusing and hard to use. In > fact, if you looked at https://people.redhat.com/sgrubb/libcap-ng/, "The > libcap-ng library is intended to make programming with posix capabilities > much easier than the traditional libcap library." > > Meanwhile, I found the go capability library interfaces are quite easy to > understand and readable. > > Jojy Varghese wrote: > I agree that libcap API is confusing. At the same time I think the the > capabilities API in docker is not well designed. An API should not be `wide`. > It should expose clean cohsive interfaces. The issue with the docker API is > that it has no clear idea of ownership. `clear`, `test` etc which are `flags` > concern, are packed along with methods that operate on a process's > credentials. > > I also believe that we dont HAVE to have a common interface called > `Capabilities` for file and process. The reason being that, process has more > functions like `drop`ing bounded caps or keeping capabilities after uid > change. > > ALso, i dont like the docker way of putting `bounded` as part of the > `type`. Since it was not part of the posix types. Thats the reason linux > kernel also has out-of-band API for it (through process control). > > Its always fun to design APIs :)
OK, what do you propose? I would like to avoid things like CapabilityFlags, CapabilitySet. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46370/#review132094 ----------------------------------------------------------- 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 > >