> 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 :)
> 
> Jie Yu wrote:
>     OK, what do you propose? I would like to avoid things like 
> CapabilityFlags, CapabilitySet.
> 
> Jie Yu wrote:
>     ALso, why keepCapabilitiesOnSetUid needs to be part of the Capability 
> interface?
> 
> Jojy Varghese wrote:
>     I propose the following:
>     
>     - Have capabilities namespace
>     - capabilities::ValueType as type for the values (enum)
>     - capabilities::Flags instead of CapabilityFlags
>     - capabilities::Sets instead of CapabilitySets
>     - capabilities::ProcessCapabilities instead of Capabilities
>     - capabilities::FileCapabilities 
>     
>     
>     Regarding `keepCapabilitiesOnSetUid`, since we are working on a process's 
> capabilities(now named ProcessCapabilities), I think it would make sense to 
> add process capabilities related functions as responsibility of 
> ProcessCapabilities class.

That does not kill 'Flags' and 'Sets'. I would like to avoid those as they are 
quite confusing. To me, I would like to have a single Capabilities class which 
contains all the details about manipulating capabilities and prefer not to 
expose Flags or Sets directly to the user (i.e., making them internal to 
ProcessCapabilities/FileCapabilities). For instance, Flags can totally be a 
hashset<Capability> or vector<Capability>, right?

Will FileCapabilities and ProcessCapabilities ever share some common methods 
(like set/test)? If yes, I would like to have a common base class for them. But 
we could punt on that for now.

BOUNDING set is tied to each thread, similar to other sets. The only difference 
is that one needs to use prctl to set it. Why do you think it's different than 
others?


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

Reply via email to