> 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.
> >     }
> >     ```

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.


- Jojy


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