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