Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

> On Aug. 11, 2016, 10:06 p.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, line 298
> > 
> >
> > This sounds important because ProcessCapabilities allows getting 
> > bounding set. Can you follow up with a patch to address this TODO?
> > 
> > Take a look at the implementation here:
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability_linux.go#L382-L417

I added https://reviews.apache.org/r/51043/ to implement this.


- Benjamin


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


On Aug. 10, 2016, 9:14 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 10, 2016, 9:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
>   src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 196
> > 
> >
> > I'd prefer this method returns `Set`.
> > 
> > We should have a general way to convert `CapabilityInfo` to 
> > `Set`, and vise versa, instead of having different return type 
> > (or parameter type) mixed in this file.
> 
> Benjamin Bannier wrote:
> If we'd just return a `Set` we'd need to use an empty set for both the 
> case of no supported capabilities, and the error case. What about using a 
> `Try` instead?

I made this a member function, as we discussed OTR. This allows us to return a 
`Set` since this approach requires a vaild `Capabilities`.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 124-134
> > 
> >
> > I'd suggest that we don't create a class `Capabilities`. Instead, let's 
> > put all methods under namesapce `capabilities`. Looks like we just need to 
> > cache `lastCapability` (`/proc/sys/kernel/cap_last_cap`), which should not 
> > change. There's no reason to keep a class for that.
> > 
> > ```
> > namespace capabilities {
> > Try initialize();
> > Try get();
> > Try set(const ProcessCapabilities& set);
> > ...
> > }
> > ```
> 
> Benjamin Bannier wrote:
> Are you suggesting we create a global variable to hold `lastCapability`? 
> It looks that one could only be set in `capabilities::initialize`, so we'd 
> require a way to denote unset/invalid state, so we'd either need something 
> like e.g., an `Option` which opens the door the nasty destruction order bugs, 
> or semantically worse, use e.g., a signed type to store an unsigned value and 
> use values < 0 to denote invalid values.
> 
> As much as I would like to get rid of an extra class here, using a class 
> to control that state appears simpler to me.

Dropping this as discussed offline.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 136-137
> > 
> >
> > I don't think this is needed. `getAllSupportedCapabilities` is 
> > sufficient.

This can indeed be removed now that `getAllSupportedCapabilities` is a member 
function.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, line 191
> > 
> >
> > As I mentioned above, this should be the `initialize` function.

Dropping this as we keep on using a `Capabilities` class instead of free 
function.s


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 171
> > 
> >
> > This function is pretty high level. I don't think it belongs here. 
> > Looking at other capability libraries like 
> > https://github.com/syndtr/gocapability/tree/master/capability, it's not 
> > there. Let's move this out of this patch first.

We discussed pulling this function out of this patch (I guess a similar 
argument could be made for `Capabilities::keepCapabilitiesOnSetUid`), and 
literally inlining them at the call sites.

While the latter could be implemented as a free function, the former requires 
`Capabilities` state, so one would either need to pass such state in, or keep 
it a member. Regard inlining, it strongly looks to me as if that way we would 
impose dealing with very low-level code on users (e.g., preparing arguments for 
`capset`). It appears as if `capabilities` or `Capabilities` might just be the 
right spot for this code; otherwise it seems to me we wouldn't provide a 
complete toolbox.

Of course it is possible to simply rip this code out for the moment, and 
potentially add it later.

What do you think?


- Benjamin


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


On Aug. 10, 2016, 9:14 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 10, 2016, 9:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

(Updated Aug. 10, 2016, 9:14 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Last changes addressing review comments.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs (updated)
-

  src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
  src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 179-180
> > 
> >
> > Do we still need this given that we only accept 
> > `_LINUX_CAPABILITY_VERSION_3`?

Good point. We only support `_LINUX_CAPABILITY_VERSION_3` so there is no need 
to store it.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 124-134
> > 
> >
> > I'd suggest that we don't create a class `Capabilities`. Instead, let's 
> > put all methods under namesapce `capabilities`. Looks like we just need to 
> > cache `lastCapability` (`/proc/sys/kernel/cap_last_cap`), which should not 
> > change. There's no reason to keep a class for that.
> > 
> > ```
> > namespace capabilities {
> > Try initialize();
> > Try get();
> > Try set(const ProcessCapabilities& set);
> > ...
> > }
> > ```

Are you suggesting we create a global variable to hold `lastCapability`? It 
looks that one could only be set in `capabilities::initialize`, so we'd require 
a way to denote unset/invalid state, so we'd either need something like e.g., 
an `Option` which opens the door the nasty destruction order bugs, or 
semantically worse, use e.g., a signed type to store an unsigned value and use 
values < 0 to denote invalid values.

As much as I would like to get rid of an extra class here, using a class to 
control that state appears simpler to me.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 196
> > 
> >
> > I'd prefer this method returns `Set`.
> > 
> > We should have a general way to convert `CapabilityInfo` to 
> > `Set`, and vise versa, instead of having different return type 
> > (or parameter type) mixed in this file.

If we'd just return a `Set` we'd need to use an empty set for both the case of 
no supported capabilities, and the error case. What about using a 
`Try` instead?


- Benjamin


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


On Aug. 10, 2016, 7:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 10, 2016, 7:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
>   src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

(Updated Aug. 10, 2016, 7:03 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Used `Set` instead of manually adjusting bitmasks in various places.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs (updated)
-

  src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
  src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50266: Introduced linux capabilities API.

[Jie Yu]

> On July 22, 2016, 12:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 34-39
> > 
> >
> > Any reason why we need to declare them here? Shouldn't 
> > `` already declared them?
> 
> Benjamin Bannier wrote:
> These are libc function mapping to sys calls which are only exposed via 
> libcap development headers. Declaring them here ourself allows us to not 
> depend on the presence of the development header files. I agree this is not 
> something we do often. Should we just introduce the dependency on the dev 
> files and do away with our own decls of these functions here?

OK. Please add more comments here (basically what you describe here).


- Jie


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


On Aug. 9, 2016, 12:30 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 9, 2016, 12:30 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
>   src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Jie Yu]

> On July 22, 2016, 12:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 108-110
> > 
> >
> > No need to for this friend unqulified function. This can just be a 
> > member function.
> 
> Benjamin Bannier wrote:
> I am not sure I follow. This provides a way to insert a 
> `ProcessCapabilities` into an `ostream` for printing, and I believe this is 
> the canonical form. If implemented as a member the use would be completely 
> different (`set.printOn(stream)`?).

Oops. My bad. Didn't realized that `<<` (unlike `+` or `-`) does not allow 
being defined as a member function.


- Jie


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


On Aug. 9, 2016, 12:30 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 9, 2016, 12:30 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
>   src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

(Updated Aug. 9, 2016, 2:30 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Another round of review comments addressed, more to come.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs (updated)
-

  src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
  src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 46-48
> > 
> >
> > No need for 'static' here if constexpr is used.
> > 
> > Also, I would prefer calling it:
> > ```
> > PROC_CAP_LAST_CAP
> > ```

I made all variables in this block `constexpr`. I am not sure how storage 
duration would be affected by using `constexpr` since this does not make these 
variable compile-time constants (i.e., the compiler might still allocate 
storage, but it could just as well optimize the variable away), but I also 
dropped the `static` specifiers.

Also, I use the name you suggested now.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, line 47
> > 
> >
> > any reason this is not a constexpr?

`constexpr` now.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, line 48
> > 
> >
> > Ditto here?

Done.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, line 257
> > 
> >
> > s/processCaps/capabilities/

Renamed here and also for the case of the parameters of `doCapSet` and `set` 
below.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 34-39
> > 
> >
> > Any reason why we need to declare them here? Shouldn't 
> > `` already declared them?

These are libc function mapping to sys calls which are only exposed via libcap 
development headers. Declaring them here ourself allows us to not depend on the 
presence of the development header files. I agree this is not something we do 
often. Should we just introduce the dependency on the dev files and do away 
with our own decls of these functions here?


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 203-212
> > 
> >
> > This does not belong here. We should put it in 
> > include/mesos/type_utils.hpp and src/common/type_utils.cpp. Please add 
> > those in a separate patch.

I moved the code and added it in https://reviews.apache.org/r/50889/.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 215-228
> > 
> >
> > Let's introduce this in a separate patch.

Moved to https://reviews.apache.org/r/50889/.


- Benjamin


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


On Aug. 8, 2016, 1:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated Aug. 8, 2016, 1:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
>   src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

(Updated Aug. 8, 2016, 1:03 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed some comments, still more work to come.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs (updated)
-

  src/CMakeLists.txt 1286ee08fe2d60867326a1f2585f054c20b52208 
  src/Makefile.am 1a9b083493612cf610b80ac5a1c11c29d6302933 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, lines 108-110
> > 
> >
> > No need to for this friend unqulified function. This can just be a 
> > member function.

I am not sure I follow. This provides a way to insert a `ProcessCapabilities` 
into an `ostream` for printing, and I believe this is the canonical form. If 
implemented as a member the use would be completely different 
(`set.printOn(stream)`?).


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 99-100
> > 
> >
> > put them in one line:
> > ```
> > default: UNREACHABLE();
> > ```

I did remove the `default` branch as we should be working on a fixed set.


> On July 22, 2016, 2:32 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 112-113
> > 
> >
> > Ditto here.

I did remove the `default` branch as we should be working on a fixed set.


- Benjamin


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


On July 24, 2016, 2:28 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50266/
> ---
> 
> (Updated July 24, 2016, 2:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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.
> 
> This patch is based on the work in https://reviews.apache.org/r/46370/.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 
>   src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50266/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

(Updated July 24, 2016, 2:28 a.m.)


Review request for mesos and Jie Yu.


Changes
---

First round of fixes for Jie's review comments.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs (updated)
-

  src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 
  src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50266: Introduced linux capabilities API.

[Jie Yu]

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



Let me know if you want to chat about any of the feedback! Thanks!


src/linux/capabilities.hpp (line 37)


I'd prefer using `int` here since protobuf enum converts to int.



src/linux/capabilities.hpp (line 81)


Let's move all global functions down (close to each other) below 
`getAllSupportedCapabilities`.



src/linux/capabilities.hpp (line 84)


Ditto on using int here.



src/linux/capabilities.hpp (lines 108 - 110)


No need to for this friend unqulified function. This can just be a member 
function.



src/linux/capabilities.hpp (line 113)


I'd prefer we don't store a bit flag here. That makes the code uncessarily 
complex. Let's hide all the bit tricky in `Capabilities` class below (as it 
interacts with syscall interface directly). Let's keep other part of the code 
simple.

Also, i'd prefer explicitly use 4 variables here intead of using an array.
```
Set effective;
Set permitted;
Set inheritable;
Set bounded;
```

That means the `get` and `set` probably need to use a `switch` statement. 
But we can kill `MAX_CAPABILITY_TYPE` which is confusing.



src/linux/capabilities.hpp (lines 117 - 119)


Let's use `/***/` style comments so that doxygen can pick them up. Ditto on 
all interfaces that you want to expose in this header.



src/linux/capabilities.hpp (lines 124 - 134)


I'd suggest that we don't create a class `Capabilities`. Instead, let's put 
all methods under namesapce `capabilities`. Looks like we just need to cache 
`lastCapability` (`/proc/sys/kernel/cap_last_cap`), which should not change. 
There's no reason to keep a class for that.

```
namespace capabilities {
Try initialize();
Try get();
Try set(const ProcessCapabilities& set);
...
}
```



src/linux/capabilities.hpp (lines 136 - 137)


I don't think this is needed. `getAllSupportedCapabilities` is sufficient.



src/linux/capabilities.hpp (lines 147 - 148)


Why the extra space before `@param` and `@return`?



src/linux/capabilities.hpp (line 171)


This function is pretty high level. I don't think it belongs here. Looking 
at other capability libraries like 
https://github.com/syndtr/gocapability/tree/master/capability, it's not there. 
Let's move this out of this patch first.



src/linux/capabilities.hpp (lines 179 - 180)


Do we still need this given that we only accept 
`_LINUX_CAPABILITY_VERSION_3`?



src/linux/capabilities.hpp (lines 187 - 190)


I would reuse `convert` for all of them:
```
Capability convert(const CapabilityInfo::Cpability& capability);
CpabilityInfo convert(const Set& capabilities);
```



src/linux/capabilities.hpp (line 196)


I'd prefer this method returns `Set`.

We should have a general way to convert `CapabilityInfo` to 
`Set`, and vise versa, instead of having different return type (or 
parameter type) mixed in this file.



src/linux/capabilities.hpp (lines 203 - 212)


This does not belong here. We should put it in include/mesos/type_utils.hpp 
and src/common/type_utils.cpp. Please add those in a separate patch.



src/linux/capabilities.hpp (lines 215 - 228)


Let's introduce this in a separate patch.



src/linux/capabilities.cpp (lines 34 - 39)


Any reason why we need to declare them here? Shouldn't `` 
already declared them?



src/linux/capabilities.cpp (lines 46 - 48)


No need for 'static' here if constexpr is used.

Also, I would prefer calling it:
```
PROC_CAP_LAST_CAP
```



src/linux/capabilities.cpp (line 47)


any reason this is not a constexpr?



src/linux/capabilities.cpp (line 48)


Ditto here?



src/linux/capabilities.cpp (line 59)


Let's move 

Review Request 50266: Introduced linux capabilities API.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


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.

This patch is based on the work in https://reviews.apache.org/r/46370/.


Diffs
-

  src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 
  src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/50266/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier