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 Mon, 09 May 2016 14:40:17 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.
> 
> 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.
> 
> Jie Yu wrote:
>     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?

We need operations like:
- Difference between two capabilities (required vs maximum or permitted) to be exposed to
the application layer
- Conversion of string representation {"NET_RAW", "SYSADMIN"...} to their flag representation
(equivalent of http://man7.org/linux/man-pages/man3/cap_from_text.3.html)
- Ability to print caps out


If we have just one class, then all this will have to be put in that class which I believe
is bad API IMO.


- 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