mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jojy Varghese <j...@mesosphere.io>
Subject Re: Review Request 46370: Introduced linux capabilities API.
Date Sat, 07 May 2016 23:37:00 GMT


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

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


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message