mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 70757: Added a privs isolator.
Date Fri, 31 May 2019 04:24:52 GMT

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



You should add a test in this review. I'd probably do something reasonable simple, like launch
a task and verify that the `NoNewPrivs` fiels in `/proc/$PID/status` is what you expect it
to be. Take a look at `src/tests/containerizer/linux_filesystem_isolator_tests.cpp` for an
example of how to construct the test by driving the containerizer directly.

We also need to add feature and upgrade documentation, but if you want to do that in a later
review, that's fine with me.


include/mesos/slave/containerizer.proto
Lines 309 (patched)
<https://reviews.apache.org/r/70757/#comment302349>

    The field number needs to be updated, since intermediate changes allocated 21.



src/slave/containerizer/mesos/isolators/linux/privs.hpp
Lines 30 (patched)
<https://reviews.apache.org/r/70757/#comment302351>

    I did originally advise you to call this isolator something more open ended, because I
had it in mind that we could extend this to toggle secbits as well.
    
    I've reconsidered and I think that I gave you bad advice. Let's check in with the other
containerization folks, but I'm inclined to call this `linux/nnp` and leave it at that.
    
    Sorry for the back & forth.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 17 (patched)
<https://reviews.apache.org/r/70757/#comment302353>

    Do you need this (once you drop the geteuid check)?



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 19 (patched)
<https://reviews.apache.org/r/70757/#comment302354>

    Do you need this?



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 24 (patched)
<https://reviews.apache.org/r/70757/#comment302356>

    I think you can remove all or most of these headers too.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 44 (patched)
<https://reviews.apache.org/r/70757/#comment302355>

    You don't need roo to set the NNP flag, so we can drop this.



src/slave/containerizer/mesos/isolators/linux/privs.cpp
Lines 49 (patched)
<https://reviews.apache.org/r/70757/#comment302352>

    We don't need this check. The NNP flag will work with any launcher, as long as the host
OS is Linux.



src/slave/containerizer/mesos/launch.cpp
Lines 1101 (patched)
<https://reviews.apache.org/r/70757/#comment302350>

    A more succinct phrasing would be:
    ```
    if (prctl(...) == -1) {
      cerr << ErrnoError("Failed to set the NO_NEW_PRIVS flag"), << endl;
      exitWithStatus(EXIT_FAILURE);
    }
    ```


- James Peach


On May 31, 2019, 4:02 a.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> -----------------------------------------------------------
> 
> (Updated May 31, 2019, 4:02 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James Peach.
> 
> 
> Bugs: MESOS-9770
>     https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b

>   src/slave/containerizer/mesos/isolators/linux/privs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/privs.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>


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