mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 59552: Add support for explicitly setting bounding capabilities.
Date Sun, 11 Jun 2017 18:11:09 GMT

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 159 (patched)
<https://reviews.apache.org/r/59552/#comment251231>

    This variable is not necessary if you follow the suggest i have in the above comment.



src/slave/containerizer/mesos/launch.cpp
Lines 642-643 (patched)
<https://reviews.apache.org/r/59552/#comment251216>

    Let's make sure inheritable is always a subset of bounding. This prevents a container
from getting permitted capabilities beyond its bounding set by crafting an executable with
proper `F(inheritable)` set.



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 91-92 (patched)
<https://reviews.apache.org/r/59552/#comment251266>

    I wouldn't do that here. I think the goal is to allow framework to specify bounding as
well in the future. I would set bounding below.
    
    Currently, framework cannot set bounding. We'll always use the operator specified bounding
if set. If not set, set bounding to effective if set.
    
    ```
    if (containerConfig.has_container_info() ...) {
      effective = ...;
    }
    
    if (effective.isNone()) {
      effective = flags.allowed_capabilities;
    }
    
    // NOTE: Currently, we do not allow framework to set
    // bounding capabilities separately. Therefore, it'll
    // always be what the operator has specified.
    bounding = flags.bounding_capabilities;
    
    // NOTE: This is for backwards compatibility.
    if (effective.isSome() && bounding.isNone()) {
      bounding = effective;
    }
    ```



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Line 93 (original), 112 (patched)
<https://reviews.apache.org/r/59552/#comment251265>

    I would use 'bounding' here instead of `flags.bounding_capabilites`. in the future, we
will allow framework to specify bounding set.



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 131 (patched)
<https://reviews.apache.org/r/59552/#comment251268>

    CHECK_SOME



src/slave/containerizer/mesos/launch.cpp
Lines 642-643 (patched)
<https://reviews.apache.org/r/59552/#comment251264>

    Style nits. I saw we have the following in the above code
    ```
    set<Capability> target = capabilities::convert(
        launchInfo.effective_capabilities());
    ```
    
    Let's make sure the style are consistent. either your way, or the previous way, but not
mixed.



src/slave/containerizer/mesos/launch.cpp
Lines 653-654 (patched)
<https://reviews.apache.org/r/59552/#comment251263>

    Style nit:
    ```
    capabilities->set(
        capabilities::INHERITABLE,
        capabilities->get(capabilities::BOUNDING));
    ```


- Jie Yu


On June 10, 2017, 9:43 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
> 
> (Updated June 10, 2017, 9:43 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
>     https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da

>   src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1

> 
> 
> Diff: https://reviews.apache.org/r/59552/diff/4/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message