-----------------------------------------------------------
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)
<https://reviews.apache.org/r/46370/#comment196187>

    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)
<https://reviews.apache.org/r/46370/#comment196238>

    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)
<https://reviews.apache.org/r/46370/#comment196240>

    When I review the code, I found very confusing. We have `CapabilityInfo, 
CapabilityInfo::Capability, Capability, Capabilities, CapabilityFlags, 
CapabilitySet`. Also, looking at the interfaces of `Capabilities`, it's not 
straightfoward to extend it to support file capabilities. If possible, I'd like 
the library here that's easily extensible to support file capabilities.
    
    In fact, I like the abstraction in the go library
    https://github.com/syndtr/gocapability/blob/master/capability/capability.go
    
    So, basically, `Capabilities` will become the struct that contains all 
capabilities (permitted, inheritted, etc) if applied. and it has methods 
defined allowing callers to modify any of them. Finally, there's an `Apply` 
method which can be used to apply the change. The methods will be virtual so 
that in the future, we can use that to support file capabilities as well:
    ```
    typedef int Capability
    
    constexpr int CHOWN = 0;
    ...
    
    enum Type
    {
      EFFECTIVE,
      PERMITTED,
      INHERITABLE,
      BOUNDING
    };
    
    class Capabilities
    {
    public:
      static Try<Owned<Capabilities>> fromPid(const Option<pid_t>& pid = 
None());
      static Try<Owned<Capabilities>> fromFile(const string& path); // TODO.
      
      virtual bool test(Type type, Capability cap);
      virtual void set(Type type, const hashset<Capability>& caps);
      virtual void unset(Type type, const hashset<Capability>& caps);
      virtual void fill(Type type);
      virtual void clear(Type type);
      
      virtual Try<Nothing> apply();
    };
    
    // Returns the set of capabilities specified in the protobuf.
    hashset<Capability> parse(const CapabilityInfo& info);
    ```
    
    In the cpp file, you can create a `class ProcessCapabilities : public 
Capabilities` that implements the above virtual methods. 
    
    For version and lastCapability, i think it can be a global variable in the 
cpp file which will be set the first time `version` is called. Any subsequent 
calling of `version` will use the cached value:
    
    ```
    Try<uint32_t> version();
    Try<Capability> lastCapability();
    
    Try<Owned<Capabilities>> ProcessCapabilities::create(const Option<pid_t>& 
pid = None())
    {
      // you can save version and lastCap in ProcessCapabilities.
    }
    ```



src/linux/capabilities.hpp (lines 267 - 282)
<https://reviews.apache.org/r/46370/#comment196192>

    Can you do that in a separate patch?



src/linux/capabilities.cpp (line 341)
<https://reviews.apache.org/r/46370/#comment196193>

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



src/linux/capabilities.cpp (line 581)
<https://reviews.apache.org/r/46370/#comment196273>

    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 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to