You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by jo...@apache.org on 2019/03/25 22:50:10 UTC
[mesos] branch 1.6.x updated (7b35929 -> 3897e09)
This is an automated email from the ASF dual-hosted git repository.
josephwu pushed a change to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from 7b35929 Fixed indentation for 1.6.3 CHANGELOG.
new a6dc0ba Exposed `memfd_create` syscall via `memfd::create` function.
new 589a713 Store `logrotate` config in memfd file instead of container's sandbox.
new 6db6752 Modified `LOGROTATE_CustomRotateOptions` test after memfd change.
new 3897e09 Added MESOS-9564 to the 1.6.3 CHANGELOG.
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
CHANGELOG | 1 +
src/linux/memfd.cpp | 12 +--
src/linux/memfd.hpp | 12 +++
src/slave/container_loggers/logrotate.cpp | 155 +++++++++++++++++++++++-------
src/tests/container_logger_tests.cpp | 60 ++++++++++--
5 files changed, 187 insertions(+), 53 deletions(-)
[mesos] 02/04: Store `logrotate` config in memfd file instead of
container's sandbox.
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
josephwu pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 589a713ff5b8c7ab4d1ea22eec6606f228747aea
Author: Andrei Budnik <ab...@mesosphere.com>
AuthorDate: Fri Feb 22 16:37:45 2019 -0800
Store `logrotate` config in memfd file instead of container's sandbox.
Previously, logrotation module stored the `logrotate` configuration
file in container's sandbox directory, so that it was garbage collected
together with the container's sandbox. If the container's task had
permissions to modify this configuration file, it was possible to run
any command under an unprivileged user. This patch stores `logrotate`
config in an nonymous temporary file via `memfd`, so logrotation module
can pass a path to procfs instead of container's sandbox. This approach
solves the aforementioned security issue on Linux when the
`ENABLE_LAUNCHER_SEALING` configuration flag is specified.
Review: https://reviews.apache.org/r/70010/
---
src/slave/container_loggers/logrotate.cpp | 155 +++++++++++++++++++++++-------
1 file changed, 118 insertions(+), 37 deletions(-)
diff --git a/src/slave/container_loggers/logrotate.cpp b/src/slave/container_loggers/logrotate.cpp
index bd41912..aa894a1 100644
--- a/src/slave/container_loggers/logrotate.cpp
+++ b/src/slave/container_loggers/logrotate.cpp
@@ -40,11 +40,20 @@
#include <stout/os/su.hpp>
#include <stout/os/write.hpp>
+// TODO(abudnik): `ENABLE_LAUNCHER_SEALING` should be replaced with
+// `__linux__`, once we drop support for kernels older than 3.17, which
+// do not support memfd.
+#ifdef ENABLE_LAUNCHER_SEALING
+#include "linux/memfd.hpp"
+#endif // ENABLE_LAUNCHER_SEALING
+
#include "logging/logging.hpp"
#include "slave/container_loggers/logrotate.hpp"
+using std::string;
+
using namespace process;
using namespace mesos::internal::logger::rotate;
@@ -52,19 +61,79 @@ using namespace mesos::internal::logger::rotate;
class LogrotateLoggerProcess : public Process<LogrotateLoggerProcess>
{
public:
- LogrotateLoggerProcess(const Flags& _flags)
- : ProcessBase(process::ID::generate("logrotate-logger")),
- flags(_flags),
- leading(None()),
- bytesWritten(0)
+ static Try<LogrotateLoggerProcess*> create(const Flags& flags)
{
- // Prepare a buffer for reading from the `incoming` pipe.
- length = os::pagesize();
- buffer = new char[length];
+ Option<int_fd> configMemFd;
+
+ // Calculate the size of the buffer that is used for reading from
+ // the `incoming` pipe.
+ const size_t bufferSize = os::pagesize();
+
+ // Populate the `logrotate` configuration file.
+ // See `Flags::logrotate_options` for the format.
+ //
+ // NOTE: We specify a size of `--max_size - bufferSize` because `logrotate`
+ // has slightly different size semantics. `logrotate` will rotate when the
+ // max size is *exceeded*. We rotate to keep files *under* the max size.
+ const string config =
+ "\"" + flags.log_filename.get() + "\" {\n" +
+ flags.logrotate_options.getOrElse("") + "\n" +
+ "size " + stringify(flags.max_size.bytes() - bufferSize) + "\n" +
+ "}";
+
+ // TODO(abudnik): `ENABLE_LAUNCHER_SEALING` should be replaced with
+ // `__linux__`, once we drop support for kernels older than 3.17, which
+ // do not support memfd.
+#ifdef ENABLE_LAUNCHER_SEALING
+ // Create a temporary anonymous file, which can be accessed by a child
+ // process by opening a `/proc/self/fd/<FD of anonymous file>`.
+ // This file is automatically removed on process termination, so we don't
+ // need to garbage collect it.
+ // We use the `memfd` file to pass the configuration to `logrotate`.
+ Try<int_fd> memFd =
+ mesos::internal::memfd::create("mesos_logrotate", 0);
+
+ if (memFd.isError()) {
+ return Error(
+ "Failed to create memfd file '" +
+ flags.log_filename.get() + "': " + memFd.error());
+ }
+
+ Try<Nothing> write = os::write(memFd.get(), config);
+ if (write.isError()) {
+ os::close(memFd.get());
+ return Error("Failed to write memfd file '" + flags.log_filename.get() +
+ "': " + write.error());
+ }
+
+ // `logrotate` requires configuration file to have 0644 or 0444 permissions.
+ int ret = fchmod(memFd.get(), S_IRUSR | S_IRGRP | S_IROTH);
+ if (ret == -1) {
+ ErrnoError error("Failed to chmod memfd file '" +
+ flags.log_filename.get() + "'");
+ os::close(memFd.get());
+ return error;
+ }
+
+ configMemFd = memFd.get();
+#else
+ Try<Nothing> result = os::write(
+ flags.log_filename.get() + CONF_SUFFIX, config);
+
+ if (result.isError()) {
+ return Error("Failed to write configuration file: " + result.error());
+ }
+#endif // ENABLE_LAUNCHER_SEALING
+
+ return new LogrotateLoggerProcess(flags, configMemFd, bufferSize);
}
virtual ~LogrotateLoggerProcess()
{
+ if (configMemFd.isSome()) {
+ os::close(configMemFd.get());
+ }
+
if (buffer != nullptr) {
delete[] buffer;
buffer = nullptr;
@@ -79,25 +148,6 @@ public:
// leading log file, and manages total log size.
Future<Nothing> run()
{
- // Populate the `logrotate` configuration file.
- // See `Flags::logrotate_options` for the format.
- //
- // NOTE: We specify a size of `--max_size - length` because `logrotate`
- // has slightly different size semantics. `logrotate` will rotate when the
- // max size is *exceeded*. We rotate to keep files *under* the max size.
- const std::string config =
- "\"" + flags.log_filename.get() + "\" {\n" +
- flags.logrotate_options.getOrElse("") + "\n" +
- "size " + stringify(flags.max_size.bytes() - length) + "\n" +
- "}";
-
- Try<Nothing> result = os::write(
- flags.log_filename.get() + CONF_SUFFIX, config);
-
- if (result.isError()) {
- return Failure("Failed to write configuration file: " + result.error());
- }
-
// NOTE: This is a prerequisuite for `io::read`.
Try<Nothing> nonblock = os::nonblock(STDIN_FILENO);
if (nonblock.isError()) {
@@ -113,7 +163,7 @@ public:
// Reads from stdin and writes to the leading log file.
void loop()
{
- io::read(STDIN_FILENO, buffer, length)
+ io::read(STDIN_FILENO, buffer, bufferSize)
.then(defer(self(), [&](size_t readSize) -> Future<Nothing> {
// Check if EOF has been reached on the input stream.
// This indicates that the container (whose logs are being
@@ -171,7 +221,7 @@ public:
// clearing the STDIN pipe (which would otherwise potentially block
// the container on write) over log fidelity.
Try<Nothing> result =
- os::write(leading.get(), std::string(buffer, readSize));
+ os::write(leading.get(), string(buffer, readSize));
if (result.isError()) {
std::cerr << "Failed to write: " << result.error() << std::endl;
@@ -198,18 +248,40 @@ public:
os::shell(
flags.logrotate_path +
" --state \"" + flags.log_filename.get() + STATE_SUFFIX + "\" \"" +
- flags.log_filename.get() + CONF_SUFFIX + "\"");
+ configPath + "\"");
// Reset the number of bytes written.
bytesWritten = 0;
}
private:
- Flags flags;
+ explicit LogrotateLoggerProcess(
+ const Flags& _flags,
+ const Option<int_fd>& _configMemFd,
+ size_t _bufferSize)
+ : ProcessBase(process::ID::generate("logrotate-logger")),
+ flags(_flags),
+ configMemFd(_configMemFd),
+ buffer(new char[_bufferSize]),
+ bufferSize(_bufferSize),
+ leading(None()),
+ bytesWritten(0)
+ {
+ if (configMemFd.isSome()) {
+ configPath = "/proc/self/fd/" + stringify(configMemFd.get());
+ } else {
+ configPath = flags.log_filename.get() + CONF_SUFFIX;
+ }
+ }
+
+ const Flags flags;
+ const Option<int_fd> configMemFd;
+
+ string configPath;
// For reading from stdin.
char* buffer;
- size_t length;
+ const size_t bufferSize;
// For writing and rotating the leading log file.
Option<int> leading;
@@ -264,15 +336,24 @@ int main(int argc, char** argv)
}
// Asynchronously control the flow and size of logs.
- LogrotateLoggerProcess process(flags);
- spawn(&process);
+ Try<LogrotateLoggerProcess*> process = LogrotateLoggerProcess::create(flags);
+ if (process.isError()) {
+ EXIT(EXIT_FAILURE)
+ << Error("Failed to create Logrotate process: " + process.error());
+ }
+
+ spawn(process.get());
// Wait for the logging process to finish.
- Future<Nothing> status = dispatch(process, &LogrotateLoggerProcess::run);
+ Future<Nothing> status =
+ dispatch(process.get(), &LogrotateLoggerProcess::run);
+
status.await();
- terminate(process);
- wait(process);
+ terminate(process.get());
+ wait(process.get());
+
+ delete process.get();
return status.isReady() ? EXIT_SUCCESS : EXIT_FAILURE;
}
[mesos] 03/04: Modified `LOGROTATE_CustomRotateOptions` test after
memfd change.
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
josephwu pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 6db67520bc10af6faee74b8eebe0248347bfefc8
Author: Andrei Budnik <ab...@mesosphere.com>
AuthorDate: Fri Feb 22 16:38:14 2019 -0800
Modified `LOGROTATE_CustomRotateOptions` test after memfd change.
This patch changes the mechanism of verifying application of custom
logrotation options. Since we don't create logrotate config file
in the container's sandbox on Linux, we can't use the previous
approach where we tried to find the custom options in the generated
`stdout.logrotate.conf`. Instead, we inject a script into custom
options, which creates an empty file in the temporary directory on
log rotation.
Review: https://reviews.apache.org/r/70011/
---
src/tests/container_logger_tests.cpp | 60 +++++++++++++++++++++++++++++++-----
1 file changed, 53 insertions(+), 7 deletions(-)
diff --git a/src/tests/container_logger_tests.cpp b/src/tests/container_logger_tests.cpp
index 2d5e7de..4c2a819 100644
--- a/src/tests/container_logger_tests.cpp
+++ b/src/tests/container_logger_tests.cpp
@@ -446,9 +446,22 @@ TEST_F(ContainerLoggerTest, LOGROTATE_CustomRotateOptions)
AWAIT_READY(offers);
ASSERT_FALSE(offers->empty());
- const string customConfig = "some-custom-logrotate-option";
+ const string testFile = sandbox.get() + "/CustomRotateOptions";
- TaskInfo task = createTask(offers.get()[0], "exit 0");
+ // Custom config consists of a postrotate script which creates
+ // an empty file in the temporary directory on log rotation.
+ const string customConfig =
+ "postrotate\n touch " + testFile + "\nendscript";
+
+ // Start a task that spams stdout with 2 MB of (mostly blank) output.
+ // The logrotate container logger module is loaded with parameters that limit
+ // the log size to five files of 2 MB each. After the task completes,
+ // `logrotate` should trigger rotation of stdout logs, so postrotate
+ // script is executed.
+ TaskInfo task = createTask(
+ offers.get()[0],
+ "i=0; while [ $i -lt 2048 ]; "
+ "do printf '%-1024d\\n' $i; i=$((i+1)); done");
// Add an override for the logger's stdout stream.
// We will check this by inspecting the generated configuration file.
@@ -480,6 +493,32 @@ TEST_F(ContainerLoggerTest, LOGROTATE_CustomRotateOptions)
driver.stop();
driver.join();
+ // The `LogrotateContainerLogger` spawns some `mesos-logrotate-logger`
+ // processes above, which continue running briefly after the container exits.
+ // Once they finish reading the container's pipe, they should exit.
+ Try<os::ProcessTree> pstrees = os::pstree(0);
+ ASSERT_SOME(pstrees);
+ foreach (const os::ProcessTree& pstree, pstrees->children) {
+ // Wait for the logger subprocesses to exit, for up to 5 seconds each.
+ Duration waited = Duration::zero();
+ do {
+ if (!os::exists(pstree.process.pid)) {
+ break;
+ }
+
+ // Push the clock ahead to speed up the reaping of subprocesses.
+ Clock::pause();
+ Clock::settle();
+ Clock::advance(Seconds(1));
+ Clock::resume();
+
+ os::sleep(Milliseconds(100));
+ waited += Milliseconds(100);
+ } while (waited < Seconds(5));
+
+ EXPECT_LE(waited, Seconds(5));
+ }
+
// Check for the expected logger files.
string sandboxDirectory = path::join(
slave::paths::getExecutorPath(
@@ -492,14 +531,21 @@ TEST_F(ContainerLoggerTest, LOGROTATE_CustomRotateOptions)
ASSERT_TRUE(os::exists(sandboxDirectory));
- // Check to see if our custom string is sitting in the configuration.
string stdoutPath = path::join(sandboxDirectory, "stdout.logrotate.conf");
- ASSERT_TRUE(os::exists(stdoutPath));
- Try<string> stdoutConfig = os::read(stdoutPath);
- ASSERT_SOME(stdoutConfig);
+ // Check that `mesos-logrotate-logger` creates config file in the container
+ // sandbox when `ENABLE_LAUNCHER_SEALING` flag is not specified. Otherwise,
+ // the config file is created in `procfs`, so it should not exist in the
+ // container sandbox.
+#ifdef ENABLE_LAUNCHER_SEALING
+ ASSERT_FALSE(os::exists(stdoutPath));
+#else
+ ASSERT_TRUE(os::exists(stdoutPath));
+#endif // ENABLE_LAUNCHER_SEALING
- ASSERT_TRUE(strings::contains(stdoutConfig.get(), customConfig));
+ // This file should be created by a script on log rotation. The script is
+ // specified via logrotate stdout options.
+ ASSERT_TRUE(os::exists(testFile));
}
[mesos] 01/04: Exposed `memfd_create` syscall via `memfd::create`
function.
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
josephwu pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit a6dc0bad1ecb1ab3a607f50e00d398c2c84c1e7a
Author: Andrei Budnik <ab...@mesosphere.com>
AuthorDate: Fri Feb 22 16:37:38 2019 -0800
Exposed `memfd_create` syscall via `memfd::create` function.
This patch exposes a wrapper function for `memfd_create` system call.
This function can be used to create an anonymous in-memory files on
Linux.
Review: https://reviews.apache.org/r/70009/
---
src/linux/memfd.cpp | 12 +++---------
src/linux/memfd.hpp | 12 ++++++++++++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/linux/memfd.cpp b/src/linux/memfd.cpp
index 348d0ce..2cd6195 100644
--- a/src/linux/memfd.cpp
+++ b/src/linux/memfd.cpp
@@ -26,15 +26,9 @@
#include <stout/os/open.hpp>
#include <stout/os/stat.hpp>
-using std::string;
-
-#if !defined(MFD_CLOEXEC)
-#define MFD_CLOEXEC 0x0001U
-#endif
+#include "linux/memfd.hpp"
-#if !defined(MFD_ALLOW_SEALING)
-#define MFD_ALLOW_SEALING 0x0002U
-#endif
+using std::string;
#if !defined(F_ADD_SEALS)
#define F_ADD_SEALS 1033
@@ -64,7 +58,7 @@ namespace mesos {
namespace internal {
namespace memfd {
-static Try<int_fd> create(const string& name, unsigned int flags)
+Try<int_fd> create(const string& name, unsigned int flags)
{
#ifdef __NR_memfd_create
int_fd fd = ::syscall(__NR_memfd_create, name.c_str(), flags);
diff --git a/src/linux/memfd.hpp b/src/linux/memfd.hpp
index 4cf9906..b2630d2 100644
--- a/src/linux/memfd.hpp
+++ b/src/linux/memfd.hpp
@@ -23,10 +23,22 @@
#include <stout/os/int_fd.hpp>
+#if !defined(MFD_CLOEXEC)
+#define MFD_CLOEXEC 0x0001U
+#endif
+
+#if !defined(MFD_ALLOW_SEALING)
+#define MFD_ALLOW_SEALING 0x0002U
+#endif
+
namespace mesos {
namespace internal {
namespace memfd {
+// Creates an anonymous in-memory file via `memfd_create`.
+Try<int_fd> create(const std::string& name, unsigned int flags);
+
+
// Clone a file into a sealed private copy such that any attempt to
// modify it will not modify the original binary. Returns the memfd of
// the sealed copy.
[mesos] 04/04: Added MESOS-9564 to the 1.6.3 CHANGELOG.
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
josephwu pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 3897e09bed71898a28f886414bd135fc49bdbba3
Author: Joseph Wu <jo...@apache.org>
AuthorDate: Mon Mar 25 15:45:50 2019 -0700
Added MESOS-9564 to the 1.6.3 CHANGELOG.
---
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
diff --git a/CHANGELOG b/CHANGELOG
index f4c4e52..4decec8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ Release Notes - Mesos - Version 1.6.3 (WIP)
** Bug
* [MESOS-9507] - Agent could not recover due to empty docker volume checkpointed files.
+ * [MESOS-9564] - Logrotate container logger lets tasks execute arbitrary commands in the Mesos agent's namespace.
** Improvement
* [MESOS-8880] - Add minimum capabilities in the master.