Repository: mesos Updated Branches: refs/heads/master 1b6f9e90f -> 04dd7f32b
Narrowed task sandbox permissions from 0755 to 0750. Since task sandboxes can contain private data, we should not make them accessible to others by default. This changes all the places that create a task sandbox directory to use a helper API `slave::paths::createSandboxDirectory` that consistently deals with setting the directory mode and ownership. A number of tests depended on the previous behavior where failing to change the ownership was logged but did not cause a failure. Depending on the test, these were updated to either disable the agent `switch_user` flag, or to specify the current user in the task launch message. Review: https://reviews.apache.org/r/64630/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/04dd7f32 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/04dd7f32 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/04dd7f32 Branch: refs/heads/master Commit: 04dd7f32b5cce29c68e20611580dc6bdc61fce11 Parents: 1b6f9e9 Author: James Peach <[email protected]> Authored: Tue Jan 16 10:29:07 2018 -0800 Committer: James Peach <[email protected]> Committed: Tue Jan 16 15:11:39 2018 -0800 ---------------------------------------------------------------------- src/slave/containerizer/mesos/containerizer.cpp | 35 +++++------- src/slave/containerizer/mesos/paths.cpp | 2 - src/slave/http.cpp | 35 +++++------- src/slave/paths.cpp | 59 +++++++++++++------- src/slave/paths.hpp | 5 ++ src/tests/api_tests.cpp | 11 +++- src/tests/master_allocator_tests.cpp | 7 ++- src/tests/master_authorization_tests.cpp | 35 +++++++++++- src/tests/slave_authorization_tests.cpp | 18 +++++- 9 files changed, 140 insertions(+), 67 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/slave/containerizer/mesos/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index ec39d04..f69f65a 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -1160,31 +1160,26 @@ Future<Containerizer::LaunchResult> MesosContainerizerProcess::launch( containers_[rootContainerId]->directory.get(), containerId); - Try<Nothing> mkdir = os::mkdir(directory); + if (containerConfig.has_user()) { + LOG_BASED_ON_CLASS(containerConfig.container_class()) + << "Creating sandbox '" << directory << "'" + << " for user '" << containerConfig.user() << "'"; + } else { + LOG_BASED_ON_CLASS(containerConfig.container_class()) + << "Creating sandbox '" << directory << "'"; + } + + Try<Nothing> mkdir = slave::paths::createSandboxDirectory( + directory, + containerConfig.has_user() ? Option<string>(containerConfig.user()) + : Option<string>::none()); + if (mkdir.isError()) { return Failure( - "Failed to create nested sandbox directory '" + + "Failed to create nested sandbox '" + directory + "': " + mkdir.error()); } -#ifndef __WINDOWS__ - if (containerConfig.has_user()) { - LOG_BASED_ON_CLASS(containerConfig.container_class()) - << "Trying to chown '" << directory << "' to user '" - << containerConfig.user() << "'"; - - Try<Nothing> chown = os::chown(containerConfig.user(), directory); - if (chown.isError()) { - LOG(WARNING) - << "Failed to chown sandbox directory '" << directory - << "'. This may be due to attempting to run the container " - << "as a nonexistent user on the agent; see the description" - << " for the `--switch_user` flag for more information: " - << chown.error(); - } - } -#endif // __WINDOWS__ - // Modify the sandbox directory in the ContainerConfig. // TODO(josephw): Should we validate that this value // is not set for nested containers? http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/slave/containerizer/mesos/paths.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/paths.cpp b/src/slave/containerizer/mesos/paths.cpp index 612c967..0df4cf5 100644 --- a/src/slave/containerizer/mesos/paths.cpp +++ b/src/slave/containerizer/mesos/paths.cpp @@ -484,8 +484,6 @@ Try<ContainerID> parseSandboxPath( return currentContainerId; } - - } // namespace paths { } // namespace containerizer { } // namespace slave { http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/slave/http.cpp ---------------------------------------------------------------------- diff --git a/src/slave/http.cpp b/src/slave/http.cpp index 71e0bbb..94fb72c 100644 --- a/src/slave/http.cpp +++ b/src/slave/http.cpp @@ -72,6 +72,7 @@ #include "slave/slave.hpp" #include "slave/validation.hpp" +#include "slave/containerizer/mesos/containerizer.hpp" #include "slave/containerizer/mesos/paths.hpp" #include "version/version.hpp" @@ -2707,29 +2708,23 @@ Future<Response> Http::_launchContainer( const string directory = slave::paths::getContainerPath(slave->flags.work_dir, containerId); - // NOTE: The below partially mirrors logic executed before the agent calls - // `containerizer->launch`. See `slave::paths::createExecutorDirectory`. - Try<Nothing> mkdir = os::mkdir(directory); - if (mkdir.isError()) { - return InternalServerError( - "Failed to create sandbox directory: " + mkdir.error()); + if (containerConfig.has_user()) { + LOG_BASED_ON_CLASS(containerConfig.container_class()) + << "Creating sandbox '" << directory << "'" + << " for user '" << containerConfig.user() << "'"; + } else { + LOG_BASED_ON_CLASS(containerConfig.container_class()) + << "Creating sandbox '" << directory << "'"; } -// `os::chown()` is not available on Windows. -#ifndef __WINDOWS__ - if (containerConfig.has_user()) { - Try<Nothing> chown = os::chown(containerConfig.user(), directory); - if (chown.isError()) { - // Attempt to clean up, but since we've already failed to chown, - // we don't check the return value here. - os::rmdir(directory); - - return InternalServerError( - "Failed to chown sandbox directory '" + - directory + "':" + chown.error()); - } + Try<Nothing> mkdir = slave::paths::createSandboxDirectory( + directory, + containerConfig.has_user() ? Option<string>(containerConfig.user()) + : Option<string>::none()); + + if (mkdir.isError()){ + return InternalServerError("Failed to create sandbox: " + mkdir.error()); } -#endif // __WINDOWS__ containerConfig.set_directory(directory); } http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/slave/paths.cpp ---------------------------------------------------------------------- diff --git a/src/slave/paths.cpp b/src/slave/paths.cpp index 9f8ee39..8a7e162 100644 --- a/src/slave/paths.cpp +++ b/src/slave/paths.cpp @@ -723,7 +723,14 @@ string createExecutorDirectory( const string directory = getExecutorRunPath(rootDir, slaveId, frameworkId, executorId, containerId); - Try<Nothing> mkdir = os::mkdir(directory); + if (user.isSome()) { + LOG(INFO) << "Creating sandbox '" << directory << "'" + << " for user '" << user.get() << "'"; + } else { + LOG(INFO) << "Creating sandbox '" << directory << "'"; + } + + Try<Nothing> mkdir = createSandboxDirectory(directory, user); CHECK_SOME(mkdir) << "Failed to create executor directory '" << directory << "'"; @@ -744,33 +751,45 @@ string createExecutorDirectory( << "Failed to symlink directory '" << directory << "' to '" << latest << "'"; -// `os::chown()` is not available on Windows. + return directory; +} + + +// Given a directory path and an optional user, create a directory +// suitable for use as a task sandbox. A task sandbox must be owned +// by the task user (if present) and have restricted permissions. +Try<Nothing> createSandboxDirectory( + const string& directory, + const Option<string>& user) +{ + Try<Nothing> mkdir = os::mkdir(directory); + if (mkdir.isError()) { + return Error("Failed to create directory: " + mkdir.error()); + } + #ifndef __WINDOWS__ + // Since this is a sandbox directory containing private task data, + // we want to ensure that it is not accessible to "others". + Try<Nothing> chmod = os::chmod(directory, 0750); + if (mkdir.isError()) { + return Error("Failed to chmod directory: " + chmod.error()); + } + if (user.isSome()) { - // Per MESOS-2592, we need to set the ownership of the executor - // directory during its creation. We should not rely on subsequent - // phases of the executor creation to ensure the ownership as - // those may be conditional and in some cases leave the executor - // directory owned by the slave user instead of the specified - // framework or per-executor user. - LOG(INFO) << "Trying to chown '" << directory << "' to user '" - << user.get() << "'"; Try<Nothing> chown = os::chown(user.get(), directory); if (chown.isError()) { - // TODO(nnielsen): We currently have tests which depend on using - // user names which may not be available on the test machines. - // Therefore, we cannot make the chown validation a hard - // CHECK(). - LOG(WARNING) << "Failed to chown executor directory '" << directory - << "'. This may be due to attempting to run the executor " - << "as a nonexistent user on the agent; see the description" - << " for the `--switch_user` flag for more information: " - << chown.error(); + // Attempt to clean up, but since we've already failed to chown, + // we don't check the return value here. + os::rmdir(directory); + + return Error( + "Failed to chown directory to '" + + user.get() + "': " + chown.error()); } } #endif // __WINDOWS__ - return directory; + return Nothing(); } http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/slave/paths.hpp ---------------------------------------------------------------------- diff --git a/src/slave/paths.hpp b/src/slave/paths.hpp index 05f826a..fff85e5 100644 --- a/src/slave/paths.hpp +++ b/src/slave/paths.hpp @@ -400,6 +400,11 @@ std::string createExecutorDirectory( const Option<std::string>& user = None()); +Try<Nothing> createSandboxDirectory( + const std::string& directory, + const Option<std::string>& user); + + std::string createSlaveDirectory( const std::string& rootDir, const SlaveID& slaveId); http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/tests/api_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/api_tests.cpp b/src/tests/api_tests.cpp index fb45879..6faefc9 100644 --- a/src/tests/api_tests.cpp +++ b/src/tests/api_tests.cpp @@ -2326,8 +2326,17 @@ TEST_P(MasterAPITest, EventAuthorizationFiltering) Future<SlaveRegisteredMessage> slaveRegisteredMessage = FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _); + // We don't need to actually launch tasks as the specified user, + // since we are only interested in testing the authorization path. + slave::Flags slaveFlags = MesosTest::CreateSlaveFlags(); + +#ifndef __WINDOWS__ + slaveFlags.switch_user = false; +#endif + Owned<MasterDetector> detector = master.get()->createDetector(); - Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), &containerizer); + Try<Owned<cluster::Slave>> slave = + StartSlave(detector.get(), &containerizer, slaveFlags); ASSERT_SOME(slave); http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/tests/master_allocator_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_allocator_tests.cpp b/src/tests/master_allocator_tests.cpp index 9bca27c..e4a6383 100644 --- a/src/tests/master_allocator_tests.cpp +++ b/src/tests/master_allocator_tests.cpp @@ -492,9 +492,14 @@ TYPED_TEST(MasterAllocatorTest, SchedulerFailover) EXPECT_CALL(sched1, resourceOffers(_, _)) .WillRepeatedly(DeclineOffers()); // For subsequent offers. + // Set the executor command user so that the task doesn't fail + // by trying to resolve the framework user `user1`. + ExecutorInfo executorInfo = DEFAULT_EXECUTOR_INFO; + executorInfo.mutable_command()->set_user(os::user().get()); + // Initially, all of the slave's resources are available. EXPECT_CALL(sched1, resourceOffers(_, OfferEq(3, 1024))) - .WillOnce(LaunchTasks(DEFAULT_EXECUTOR_INFO, 1, 1, 256, "*")); + .WillOnce(LaunchTasks(executorInfo, 1, 1, 256, "*")); // We don't filter the unused resources to make sure that // they get offered to the framework as soon as it fails over. http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/tests/master_authorization_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_authorization_tests.cpp b/src/tests/master_authorization_tests.cpp index 676543a..d06a50e 100644 --- a/src/tests/master_authorization_tests.cpp +++ b/src/tests/master_authorization_tests.cpp @@ -91,8 +91,23 @@ namespace mesos { namespace internal { namespace tests { +class MasterAuthorizationTest : public MesosTest +{ +protected: + slave::Flags CreateSlaveFlags() override + { + slave::Flags flags = MesosTest::CreateSlaveFlags(); + +#ifndef __WINDOWS__ + // We don't need to actually launch tasks as the specified + // user, since we are only interested in testing the + // authorization path. + flags.switch_user = false; +#endif -class MasterAuthorizationTest : public MesosTest {}; + return flags; + } +}; // This test verifies that an authorized task launch is successful. @@ -1321,7 +1336,23 @@ TEST_F(MasterAuthorizationTest, FrameworkRemovedBeforeReregistration) template <typename T> -class MasterAuthorizerTest : public MesosTest {}; +class MasterAuthorizerTest : public MesosTest +{ +protected: + slave::Flags CreateSlaveFlags() override + { + slave::Flags flags = MesosTest::CreateSlaveFlags(); + +#ifndef __WINDOWS__ + // We don't need to actually launch tasks as the specified + // user, since we are only interested in testing the + // authorization path. + flags.switch_user = false; +#endif + + return flags; + } +}; typedef ::testing::Types< http://git-wip-us.apache.org/repos/asf/mesos/blob/04dd7f32/src/tests/slave_authorization_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/slave_authorization_tests.cpp b/src/tests/slave_authorization_tests.cpp index 4ba0b8e..f3848db 100644 --- a/src/tests/slave_authorization_tests.cpp +++ b/src/tests/slave_authorization_tests.cpp @@ -85,7 +85,23 @@ namespace internal { namespace tests { template <typename T> -class SlaveAuthorizerTest : public MesosTest {}; +class SlaveAuthorizerTest : public MesosTest +{ +protected: + slave::Flags CreateSlaveFlags() override + { + slave::Flags flags = MesosTest::CreateSlaveFlags(); + +#ifndef __WINDOWS__ + // We don't need to actually launch tasks as the specified + // user, since we are only interested in testing the + // authorization path. + flags.switch_user = false; +#endif + + return flags; + } +}; typedef ::testing::Types<
