Re: Review Request 46370: Introduced linux capabilities API.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review148878
---



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 26, 2016, 3:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated May 26, 2016, 3:01 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 e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 26, 2016, 3:01 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 (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 25, 2016, 6:38 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

review addressed.


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 (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review133860
---




src/linux/capabilities.hpp (line 37)


As I mentioned earlier, can you specify the enum base as well?
http://en.cppreference.com/w/cpp/language/enum



src/linux/capabilities.hpp (line 84)


Let's move this too the end of this file.



src/linux/capabilities.hpp (line 107)


s/index/type/



src/linux/capabilities.cpp (line 48)


constexpr char
I would kill empty lines between those constants.



src/linux/capabilities.cpp (lines 58 - 60)


Not needed.



src/linux/capabilities.cpp (line 69)


I would suggest that we do not use a union here. Instead, use `struct 
__user_cap_data_struct data` directly.



src/linux/capabilities.cpp (line 70)


`s/__u32/uint32_t/



src/linux/capabilities.cpp (lines 75 - 90)


Let's inline those helpers and make SyscallPayload a pure struct.



src/linux/capabilities.cpp (lines 93 - 111)


Is it being used? Why do we need that?



src/linux/capabilities.cpp (line 116)


I would follow the following style:
```
switch (cap) {
  case CHOWN: return stream << "CHOWN";
  case DAC_OVERRIDE: return stream << "DAC_OVERRIDE";
  ...
  default:
UNREACHABLE();
}
```



src/linux/capabilities.cpp (lines 354 - 370)


Same here.



src/linux/capabilities.cpp (lines 545 - 547)


Making 'sets[x]' here Option is confusing. Can you make it non optional?



src/linux/capabilities.cpp (lines 555 - 570)


The check here will be performed by the kernel. I would say let's just rely 
on kernel to do the check, and get rid of the redundent check here.



src/linux/capabilities.cpp (lines 606 - 639)


Similarily, let's make this method very simple and it's callers 
responsibility to make sure it has proper capability to do prctl here.


- Jie Yu


On May 17, 2016, 2:59 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated May 17, 2016, 2:59 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 e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 17, 2016, 2:59 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 (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 13, 2016, 12:45 a.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 (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 11, 2016, 4:34 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

review addressed.


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 (updated)
-

  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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.

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.


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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > 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 fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset 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 version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& 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 

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review132094
---



See my detailed comments. I think we're introducing too many classes in this 
library code, making it very confusing.


src/linux/capabilities.hpp (lines 37 - 39)


I would suggest that we put all capability related stuff in 
mesos::internal::capability namespace.

Since we'll be converting from enum to int constantly, i would prefer we 
use `int` and `constexpr` for those constants.

```
constexpr int CHOWN = 0;
constexpr int DAC_override = 1;
...
```

The callsites read well:
```
capability::SYS_ADMIN
```



src/linux/capabilities.hpp (line 88)


what about BOUNDING? Do we need to get that as well? Looking at the go 
capability library that docker uses:
https://github.com/syndtr/gocapability/blob/master/capability/enum.go#L31



src/linux/capabilities.hpp (line 194)


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 fromPid(const Option& pid = 
None());
  static Try fromFile(const string& path); // TODO.
  
  virtual bool test(Type type, Capability cap);
  virtual void set(Type type, const hashset& caps);
  virtual void unset(Type type, const hashset& caps);
  virtual void fill(Type type);
  virtual void clear(Type type);
  
  virtual Try apply();
};

// Returns the set of capabilities specified in the protobuf.
hashset 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 version();
Try lastCapability();

Try ProcessCapabilities::create(const Option& 
pid = None())
{
  // you can save version and lastCap in ProcessCapabilities.
}
```



src/linux/capabilities.hpp (lines 267 - 282)


Can you do that in a separate patch?



src/linux/capabilities.cpp (line 341)


I think 'version' is a 32 bit unsigned integer. Can you use uint32_t 
instead?



src/linux/capabilities.cpp (line 581)


Why we allow caller to specify a cap as a string? Are you worried about the 
parsing? Can we just allow operator to specify a JSON of type `CapabilityInfo`?


- Jie Yu


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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
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 (updated)
-

  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated May 5, 2016, 7:42 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 (updated)
-

  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated April 28, 2016, 9:44 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 (updated)
-

  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated April 26, 2016, 1:23 a.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 (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > I added a few comments below, but in general, I feel like there are places 
> > this code could be greatly simplified.  Specifically, it's not obvious to 
> > me why we need all of the different classes you define (or maybe more about 
> > why they need to be part of the public interface).  Could you clarify the 
> > rational here a bit?

Classes like `CapabilityFlags` allows good encapsulation of the bitmap and 
makes the API more fiendly to deal with. Leaving them as bitmap would leave the 
complexity to the caller.

Same idea with other classes.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review129674
---


On April 20, 2016, 8:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 20, 2016, 8:19 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 8e3eecb172e6c6b5256313f0937dc1156e91 
>   src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.cpp, lines 124-125
> > 
> >
> > This should be unnecessary. See: 
> > https://github.com/klueska-mesosphere/mesos/blob/master/src/linux/cgroups.cpp#L2435

Not sure about this as  I like to lean towards good programming practice than 
depending on compiler flags.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review129674
---


On April 20, 2016, 8:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 20, 2016, 8:19 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 8e3eecb172e6c6b5256313f0937dc1156e91 
>   src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated April 20, 2016, 8:19 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 (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

(Updated April 20, 2016, 7:05 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 (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  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

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 94-99
> > 
> >
> > From my reading of: 
> > http://man7.org/linux/man-pages/man7/capabilities.7.html
> > 
> > this enum should probably be called `Set`.
> > 
> > Note, the name `Capability` at the front is unnecessary if we embed 
> > this in the `capabilities` namespace.
> > 
> > Also, it's pretty standard practice in C++ to define an `enum` as a 
> > `enum class` for better type checking.  As such, you can define the final 
> > element with a common name of `COUNT` to get at the size of the enum.
> > 
> > For example, you can get at the size of the enum as: 
> > `capabilities::Set::COUNT` instead of relying on the `const` for 
> > `NUMBER_OF_CAP_SETS` defined above.
> 
> Jojy Varghese wrote:
> Although i agree that C++11 supports enum classes, couple of reasons for 
> using plain enum here:
> 1. Other places in the code mostly use plain old enums.
> 2. Its hard(not impossible) to get the value of the enum class's element 
> (say for printing).

regarding naming, If you look at the code of libcap (a standard portable 
capability interface) 
(https://github.com/abstrakraft/lxc-android-libcap/blob/master/libcap/libcap.h),
 the name `set` is referenced for the value and not the type. In fact the 
`SystemCallPayload` structure in my code is same from libcap.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review129674
---


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 19, 2016, 5:02 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 ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 32-33
> > 
> >
> > This should all probably live in the mesos::internal::capabilities 
> > namespace.

The reasons why I chose to have it in mesos::internal is that we have 
Capabilities class (explained in the class's documentation why we need a class).


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 94-99
> > 
> >
> > From my reading of: 
> > http://man7.org/linux/man-pages/man7/capabilities.7.html
> > 
> > this enum should probably be called `Set`.
> > 
> > Note, the name `Capability` at the front is unnecessary if we embed 
> > this in the `capabilities` namespace.
> > 
> > Also, it's pretty standard practice in C++ to define an `enum` as a 
> > `enum class` for better type checking.  As such, you can define the final 
> > element with a common name of `COUNT` to get at the size of the enum.
> > 
> > For example, you can get at the size of the enum as: 
> > `capabilities::Set::COUNT` instead of relying on the `const` for 
> > `NUMBER_OF_CAP_SETS` defined above.

Although i agree that C++11 supports enum classes, couple of reasons for using 
plain enum here:
1. Other places in the code mostly use plain old enums.
2. Its hard(not impossible) to get the value of the enum class's element (say 
for printing).


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, line 178
> > 
> >
> > Didn't we discuss not making this a class, and only having get()/set() 
> > calls as part of the namespace?

Explained in the class's documentation.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, line 209
> > 
> >
> > What did we decide about the `add()` pairing to this `drop()` call?

As mentioned in the documentation of `drop`, the `drop` API is for dropping 
`bounding` capabilities.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.cpp, lines 36-38
> > 
> >
> > Is there not a header file you can just include here?

No the standard header files dont provide the syscall declaration.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 50-90
> > 
> >
> > Since we should probably be embedding this in a `capabilities` 
> > namespace, it is redundant to call this enum `Capability`. I'd sugggest 
> > `Privilege`.  That way one of these can be accessed as e.g. 
> > `capabiliites::Privilege::SETGID`.
> > 
> > Also, as mentioned in a comment below, this should probably be declared 
> > as an `enum class` for better type checking.
> > 
> > The `COUNT` trick mentioned below should probably be applied here as 
> > well.

I like Capability because that is what its referenced as in every documentation 
and literature.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review129674
---


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 19, 2016, 5:02 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 ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   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
> 
>

Re: Review Request 46370: Introduced linux capabilities API.

[Kevin Klues]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/#review129674
---



I added a few comments below, but in general, I feel like there are places this 
code could be greatly simplified.  Specifically, it's not obvious to me why we 
need all of the different classes you define (or maybe more about why they need 
to be part of the public interface).  Could you clarify the rational here a bit?


src/linux/capabilities.hpp (line 30)


Is this file #including itself? Does this compile?



src/linux/capabilities.hpp (lines 32 - 33)


This should all probably live in the mesos::internal::capabilities 
namespace.



src/linux/capabilities.hpp (lines 39 - 45)


These are both probably unnecessary (see comments below).



src/linux/capabilities.hpp (lines 50 - 90)


Since we should probably be embedding this in a `capabilities` namespace, 
it is redundant to call this enum `Capability`. I'd sugggest `Privilege`.  That 
way one of these can be accessed as e.g. `capabiliites::Privilege::SETGID`.

Also, as mentioned in a comment below, this should probably be declared as 
an `enum class` for better type checking.

The `COUNT` trick mentioned below should probably be applied here as well.



src/linux/capabilities.hpp (lines 94 - 99)


From my reading of: http://man7.org/linux/man-pages/man7/capabilities.7.html

this enum should probably be called `Set`.

Note, the name `Capability` at the front is unnecessary if we embed this in 
the `capabilities` namespace.

Also, it's pretty standard practice in C++ to define an `enum` as a `enum 
class` for better type checking.  As such, you can define the final element 
with a common name of `COUNT` to get at the size of the enum.

For example, you can get at the size of the enum as: 
`capabilities::Set::COUNT` instead of relying on the `const` for 
`NUMBER_OF_CAP_SETS` defined above.



src/linux/capabilities.hpp (line 178)


Didn't we discuss not making this a class, and only having get()/set() 
calls as part of the namespace?



src/linux/capabilities.hpp (line 209)


What did we decide about the `add()` pairing to this `drop()` call?



src/linux/capabilities.cpp (lines 36 - 38)


Is there not a header file you can just include here?



src/linux/capabilities.cpp (lines 124 - 125)


This should be unnecessary. See: 
https://github.com/klueska-mesosphere/mesos/blob/master/src/linux/cgroups.cpp#L2435


- Kevin Klues


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 19, 2016, 5:02 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 ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   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
> 
>

Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46370/
---

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 ca59a1824352423f4db6ef8bb41acc6fe602c041 
  src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
  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