[ML] PR E: typed routing behind the established controller protocol

.buildkite/scripts/steps/run_tests.sh

@@ -49,12 +49,21 @@ TEST_OUTCOME=0
 
 if [[ "$HARDWARE_ARCH" = aarch64 && -z "${CPP_CROSS_COMPILE:-}" && "$(uname)" = Linux ]]; then
     # --- Linux aarch64: run tests inside Docker container from base image ---
+    # aarch64 Buildkite k8s pods are the only runners here with userns
+    # capability (mount("proc", ...) succeeds), so this is the only branch
+    # that can exercise ML_SANDBOX2_REQUIRE=enforced - and it runs only that
+    # mode: aarch64 is pinned to enforced, x86_64 stays fail-closed. A
+    # second fail_closed pass on this same host/kernel would assert the
+    # absence of the very userns capability the enforced pass just proved
+    # present, so exactly one of the two could ever pass.
+    export ML_SANDBOX2_REQUIRE=enforced
+
     BASE_IMAGE="docker.elastic.co/ml-dev/ml-linux-aarch64-native-build:17"
 
 . ./dev-tools/docker/prefetch_docker_image.sh
     prefetch_docker_image "$BASE_IMAGE"
 
-    echo "--- Running tests (Docker)"
+    echo "--- Running tests (Docker, ML_SANDBOX2_REQUIRE=${ML_SANDBOX2_REQUIRE})"
     docker run --rm \
         -v "$(pwd)/${BUILD_DIR}:/ml-cpp/${BUILD_DIR}" \
         -v "$(pwd)/build:/ml-cpp/build" \
@@ -64,6 +73,7 @@ if [[ "$HARDWARE_ARCH" = aarch64 && -z "${CPP_CROSS_COMPILE:-}" && "$(uname)" =
         -v "$(pwd)/set_env.sh:/ml-cpp/set_env.sh:ro" \
         -v "$(pwd)/gradle.properties:/ml-cpp/gradle.properties:ro" \
         -e BOOST_TEST_OUTPUT_FORMAT_FLAGS="${BOOST_TEST_OUTPUT_FORMAT_FLAGS:-}" \
+        -e ML_SANDBOX2_REQUIRE="${ML_SANDBOX2_REQUIRE}" \
         ${TEST_TIMEOUT:+-e TEST_TIMEOUT="${TEST_TIMEOUT}"} \
         -w /ml-cpp \
         $BASE_IMAGE bash -c '
@@ -87,6 +97,15 @@ if [[ "$HARDWARE_ARCH" = aarch64 && -z "${CPP_CROSS_COMPILE:-}" && "$(uname)" =
 
 else
     # --- Linux x86_64 / macOS: run tests directly ---
+    # x86_64 Buildkite k8s pods get EPERM on mount("proc", ...) - there is no
+    # userns-capable x86_64 CI runner today, so this is an accepted gap in
+    # enforced-mode coverage on that architecture. Only fail_closed runs
+    # here; do not add an enforced pass to this branch. This
+    # also covers aarch64 cross-compile builds, which fall through to this
+    # same branch via the "-z ${CPP_CROSS_COMPILE:-}" condition above, so
+    # they get fail_closed coverage too rather than being skipped entirely.
+    export ML_SANDBOX2_REQUIRE=fail_closed
+
     . ./set_env.sh
 
     find ${BUILD_DIR}/test -name "ml_test_*" -type f -exec chmod +x {} \;
@@ -101,7 +120,7 @@ else
         export DYLD_LIBRARY_PATH="${LIB_DIRS}${DYLD_LIBRARY_PATH:+:$DYLD_LIBRARY_PATH}"
     fi
 
-    echo "--- Running tests"
+    echo "--- Running tests (ML_SANDBOX2_REQUIRE=${ML_SANDBOX2_REQUIRE})"
     cmake \
         -DSOURCE_DIR="$(pwd)" \
         -DBUILD_DIR="$(pwd)/${BUILD_DIR}" \

3rd_party/controller-protocol.version

@@ -0,0 +1 @@
+controller-protocol-version=2

bin/controller/CCommandProcessor.cc

@@ -15,11 +15,26 @@
 #include <core/CStringUtils.h>
 
 #include <algorithm>
+#include <cstdlib>
 #include <istream>
+#include <string>
 
 namespace {
 const std::string TAB(1, '\t');
 const std::string EMPTY_STRING;
+//! Operator kill-switch: forces the legacy route for the configured
+//! sandboxed process path. Mutually exclusive with REQUIRE_SANDBOX_TOKEN -
+//! a start command naming both is ambiguous about its own route and is
+//! rejected outright, never resolved by precedence.
+const std::string DISABLE_SANDBOX_TOKEN{"--disableSandbox"};
+
+//! Operator opt-in: forces the Sandbox2 route (E_Sandbox2, no automatic
+//! legacy fallback) for the configured sandboxed process path. Symmetric
+//! counterpart to DISABLE_SANDBOX_TOKEN - together these are the only two
+//! controller-control tokens the command wire format defines; any other
+//! unrecognised "--" prefixed token is passed through to the spawned
+//! process unchanged.
+const std::string REQUIRE_SANDBOX_TOKEN{"--requireSandbox"};
 }
 
 namespace ml {
@@ -30,8 +45,9 @@ const std::string CCommandProcessor::START{"start"};
 const std::string CCommandProcessor::KILL{"kill"};
 
 CCommandProcessor::CCommandProcessor(const TStrVec& permittedProcessPaths,
+                                     const TStrVec& sandboxedProcessPaths,
                                      std::ostream& responseStream)
-    : m_Spawner{permittedProcessPaths}, m_ResponseWriter{responseStream} {
+    : m_Spawner{permittedProcessPaths, sandboxedProcessPaths}, m_ResponseWriter{responseStream} {
 }
 
 void CCommandProcessor::processCommands(std::istream& commandStream) {
@@ -92,7 +108,131 @@ bool CCommandProcessor::handleStart(std::uint32_t id, TStrVec tokens) {
     std::string processPath{std::move(tokens[0])};
     tokens.erase(tokens.begin());
 
-    if (m_Spawner.spawn(processPath, tokens) == false) {
+    // Scan for both routing tokens before any spawn decision is made.
+    // Never "last one wins"/"first one wins" on duplicates of either token -
+    // count them all and reject outright if either appears more than once.
+    std::size_t disableSandboxCount{0};
+    TStrVec::iterator firstDisableSandbox{tokens.end()};
+    std::size_t requireSandboxCount{0};
+    TStrVec::iterator firstRequireSandbox{tokens.end()};
+    for (auto iter = tokens.begin(); iter != tokens.end(); ++iter) {
+        if (*iter == DISABLE_SANDBOX_TOKEN) {
+            if (disableSandboxCount == 0) {
+                firstDisableSandbox = iter;
+            }
+            ++disableSandboxCount;
+        } else if (*iter == REQUIRE_SANDBOX_TOKEN) {
+            if (requireSandboxCount == 0) {
+                firstRequireSandbox = iter;
+            }
+            ++requireSandboxCount;
+        }
+    }
+
+    if (disableSandboxCount >= 2) {
+        std::string error{"Rejecting command: '" + DISABLE_SANDBOX_TOKEN + "' specified " +
+                          core::CStringUtils::typeToString(disableSandboxCount) +
+                          " times for process '" + processPath + '\''};
+        LOG_ERROR(<< error << " in command with ID " << id);
+        m_ResponseWriter.writeResponse(id, false, error);
+        return false;
+    }
+
+    if (requireSandboxCount >= 2) {
+        std::string error{"Rejecting command: '" + REQUIRE_SANDBOX_TOKEN + "' specified " +
+                          core::CStringUtils::typeToString(requireSandboxCount) +
+                          " times for process '" + processPath + '\''};
+        LOG_ERROR(<< error << " in command with ID " << id);
+        m_ResponseWriter.writeResponse(id, false, error);
+        return false;
+    }
+
+    if (disableSandboxCount == 1 && requireSandboxCount == 1) {
+        std::string error{"Rejecting command: '" + DISABLE_SANDBOX_TOKEN +
+                          "' and '" + REQUIRE_SANDBOX_TOKEN +
+                          "' are mutually exclusive, both specified for process '" +
+                          processPath + '\''};
+        LOG_ERROR(<< error << " in command with ID " << id);
+        m_ResponseWriter.writeResponse(id, false, error);
+        return false;
+    }
+
+    // One shared predicate with the router (which uses the same call to gate
+    // dispatch and sandbox2_launch-signal emission), never a second std::find over a
+    // second copy of the list.
+    const bool isConfiguredSandboxedPath{m_Spawner.isSandboxedProcessPath(processPath)};
+
+    CProcessSpawnerRouter::ERoute route{CProcessSpawnerRouter::ERoute::E_Sandbox2};
+    // Provenance of a legacy route, recorded at the one place it is known so
+    // the router's sandbox2_launch signal can report it as "legacy_reason". Stays
+    // E_NotLegacy for every E_Sandbox2 route, where the field is omitted.
+    CProcessSpawnerRouter::ELegacyReason legacyReason{
+        CProcessSpawnerRouter::ELegacyReason::E_NotLegacy};
+    if (requireSandboxCount == 1) {
+        if (isConfiguredSandboxedPath == false) {
+            std::string error{"Rejecting command: '" + REQUIRE_SANDBOX_TOKEN +
+                              "' is only valid for the configured sandboxed process, "
+                              "not '" +
+                              processPath + '\''};
+            LOG_ERROR(<< error << " in command with ID " << id);
+            m_ResponseWriter.writeResponse(id, false, error);
+            return false;
+        }
+
+        // Operator opt-in validated against this exact processPath: strip
+        // it before it reaches the spawner. Route is already E_Sandbox2
+        // (the default above), so nothing else changes here beyond
+        // stripping and logging the decision at the one place its
+        // provenance is known.
+        LOG_INFO(<< "Routing '" << processPath << "' to Sandbox2: operator opt-in "
+                 << REQUIRE_SANDBOX_TOKEN << " in command with ID " << id);
+        tokens.erase(firstRequireSandbox);
+    } else if (disableSandboxCount == 1) {
+        if (isConfiguredSandboxedPath == false) {
+            std::string error{"Rejecting command: '" + DISABLE_SANDBOX_TOKEN +
+                              "' is only valid for the configured sandboxed process, "
+                              "not '" +
+                              processPath + '\''};
+            LOG_ERROR(<< error << " in command with ID " << id);
+            m_ResponseWriter.writeResponse(id, false, error);
+            return false;
+        }
+
+        // Operator kill-switch validated against this exact processPath:
+        // strip it before it reaches the spawner and route to legacy. This
+        // is the one place the route's operator provenance is known, so it
+        // is logged here rather than in the router, which only ever sees an
+        // already-decided route.
+        LOG_INFO(<< "Routing '" << processPath << "' to the legacy path: operator kill switch "
+                 << DISABLE_SANDBOX_TOKEN << " in command with ID " << id);
+        route = CProcessSpawnerRouter::ERoute::E_Legacy;
+        legacyReason = CProcessSpawnerRouter::ELegacyReason::E_KillSwitch;
+        tokens.erase(firstDisableSandbox);
+    } else {
+        // No token at all: the route is only a decision at all for a
+        // configured sandboxed process path (every other permitted process
+        // dispatches to the legacy spawner either way, and must not be
+        // described as an explicitly-selected legacy route in the log).
+        //
+        // Permanent behaviour, not a rollout seam: a caller that sends
+        // neither token always takes the legacy route - byte-for-byte the
+        // pre-typed-routing behaviour on every platform, including builds
+        // with no Sandbox2 support at all. Elasticsearch is expected to
+        // always send exactly one of the two tokens on every start command
+        // for a sandboxed-eligible process, so this branch exists for
+        // non-ES callers (support/debug scripts, direct controller
+        // invocation) and the test harness.
+        if (isConfiguredSandboxedPath) {
+            route = CProcessSpawnerRouter::ERoute::E_Legacy;
+            legacyReason = CProcessSpawnerRouter::ELegacyReason::E_NoTokenDefault;
+            LOG_DEBUG(<< "Routing '" << processPath << "' to the legacy path: neither "
+                      << DISABLE_SANDBOX_TOKEN << " nor "
+                      << REQUIRE_SANDBOX_TOKEN << " token was present");
+        }
+    }
+
+    core::CProcess::TPid childPid{0};
+    if (m_Spawner.spawn(route, processPath, tokens, childPid, legacyReason) == false) {
         std::string error{"Failed to start process '" + processPath + '\''};
         LOG_ERROR(<< error << " in command with ID " << id);
         m_ResponseWriter.writeResponse(id, false, error);

bin/controller/CCommandProcessor.h

@@ -11,8 +11,7 @@
 #ifndef INCLUDED_ml_controller_CCommandProcessor_h
 #define INCLUDED_ml_controller_CCommandProcessor_h
 
-#include <core/CDetachedProcessSpawner.h>
-
+#include "CProcessSpawnerRouter.h"
 #include "CResponseJsonWriter.h"
 
 #include <cstdint>
@@ -63,7 +62,16 @@ class CCommandProcessor {
     static const std::string KILL;
 
 public:
-    CCommandProcessor(const TStrVec& permittedProcessPaths, std::ostream& responseStream);
+    //! \param permittedProcessPaths Processes that may be started/killed.
+    //! \param sandboxedProcessPaths Subset of \p permittedProcessPaths for
+    //!        which the operator kill-switch token (\c --disableSandbox) is
+    //!        meaningful. Pass an explicit (possibly empty) list - there is
+    //!        no default that reuses \p permittedProcessPaths, because doing
+    //!        so would silently make every permitted process
+    //!        sandboxed-eligible.
+    CCommandProcessor(const TStrVec& permittedProcessPaths,
+                      const TStrVec& sandboxedProcessPaths,
+                      std::ostream& responseStream);
 
     //! Action commands read from the supplied \p commandStream until
     //! end-of-file is reached.
@@ -85,8 +93,12 @@ class CCommandProcessor {
     bool handleKill(std::uint32_t id, TStrVec tokens);
 
 private:
-    //! Used to spawn/kill the requested processes.
-    core::CDetachedProcessSpawner m_Spawner;
+    //! Used to spawn/kill the requested processes, and the single owner of
+    //! the "is this a configured sandboxed process path" predicate this
+    //! class queries via CProcessSpawnerRouter::isSandboxedProcessPath()
+    //! rather than keeping its own second copy of the list and the
+    //! std::find over it.
+    CProcessSpawnerRouter m_Spawner;
 
     //! Used to write responses in JSON format to the response stream.
     CResponseJsonWriter m_ResponseWriter;

bin/controller/CMakeLists.txt

@@ -11,9 +11,10 @@
 
 project("ML Controller")
 
-set(ML_LINK_LIBRARIES 
+set(ML_LINK_LIBRARIES
   ${Boost_LIBRARIES}
   MlCore
+  MlSandbox
   MlSeccomp
   MlVer
   )
@@ -22,5 +23,27 @@ ml_add_executable(controller
   CBlockingCallCancellingStreamMonitor.cc
   CCmdLineParser.cc
   CCommandProcessor.cc
+  CProcessSpawnerRouter.cc
   CResponseJsonWriter.cc
   )
+
+# ml_add_executable() also creates an OBJECT library (Mlcontroller) holding
+# the sources above, purely so bin/controller/unittest can link the same
+# object files as the executable. That OBJECT library has no link libraries
+# of its own, so - unlike the `controller` executable target - it does not
+# inherit MlSandbox's usage requirements, and in particular does not see
+# MlSandbox's PUBLIC SANDBOX2_AVAILABLE compile definition. The unit test
+# executable *does* link MlSandbox and therefore does see it, so without
+# this line ml_test_controller mixes two different views of
+# include/sandbox/CSandboxedProcessSpawner.h in one binary: that header
+# declares one extra member (the m_AwaitResultFn seam) under
+# SANDBOX2_AVAILABLE, so sizeof(CSandboxedProcessSpawner) - and hence
+# sizeof(CProcessSpawnerRouter) and sizeof(CCommandProcessor) - differ
+# between the object files and the test translation units. That is an ODR
+# violation, and it corrupted memory during test teardown on Linux.
+# Link the OBJECT library against MlSandbox so its sources are compiled
+# with exactly the same Sandbox2 configuration as both the production
+# executable and the unit tests.
+if(TARGET Mlcontroller)
+  target_link_libraries(Mlcontroller PRIVATE MlSandbox)
+endif()