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