Repository navigation
[ML] Typed routing behind the established controller protocol - #3188
Conversation
The check hard-fails bundlePlugin/explodedBundlePlugin today because the resolved ml-cpp SNAPSHOT does not yet publish controller-protocol.version (companion change: elastic/ml-cpp#3188). That blocks every build on this branch, including work unrelated to sandboxing. sandbox_enabled defaults to false during this dark-launch window, so nothing downstream depends on the mismatch yet. Log a warning instead of throwing for now; revert to GradleException once ml-cpp#3188 lands and mlCppVersion() resolves to an artifact containing the file. Detection logic is unchanged.
…ocked The javadoc claimed the sandbox_enabled=false (default) path "uses exactly the pipe layout the bundled ml-cpp controller already supports today" and was therefore independent of the paired ml-cpp artifact. That's false: PyTorchBuilder emits --disableSandbox unconditionally on Linux at sandbox_enabled=false (PyTorchBuilder#buildCommand), and today's bundled controller has no parsing for that token - it forwards it verbatim to pytorch_inference, which aborts on the unrecognized CLI option. So the default path is blocked on the paired artifact too, for a different reason (unrecognized flag) than the explicit-enable path (missing isolated IPC directory support). Rewrite the class-level javadoc and the per-method javadoc on testDefaultSandboxDisabledStartsAndInfersSuccessfully and testChildIpcPathsIsolatedPerDeployment (both run at the default setting) to state plainly that every test method in this class is currently blocked on elastic/ml-cpp#3188, and add @AwaitsFix to those two methods for consistency with the other two - verifyControllerProtocolVersion now hard-fails the build before any of these tests could run regardless of annotation, so all four should carry the same accurate, skip-cleanly-not-error-loudly annotation.
|
Pinging @elastic/ml-core (Team:ML) |
f1bb15c to
3ffdca8
Compare
3ffdca8 to
bc2e6a9
Compare
bc2e6a9 to
a74b251
Compare
a74b251 to
1765d61
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The attack-defense harness currently destroys its inference binary and loses FIFO readers before child writers connect, with additional IPC, Windows, probe, and Sandbox2-route issues identified.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Adds typed Sandbox2/legacy routing for pytorch_inference, strengthens IPC/filesystem policy handling, and wires observability plus CI coverage.
Changes:
- Adds symmetric routing tokens and fail-closed dispatch.
- Extends Sandbox2 syscall,
/proc, IPC, and environment protections. - Adds tests, attack-defense tooling, documentation, and protocol metadata.
| File | Description |
|---|---|
test/test_sandbox2_attack_defense.py |
End-to-end attack-defense harness |
test/evil_model_generator.py |
Generates test models |
lib/seccomp/unittest/CSeccompFilterBuilderTest.cc |
Seccomp regression tests |
lib/sandbox/unittest/payloads/ml_sandbox_userns_probe.cc |
User-namespace capability probe |
lib/sandbox/unittest/payloads/ml_sandbox_probe.cc |
Adds /proc/self/exe probe |
lib/sandbox/unittest/payloads/lifecycle_signal_payload.cc |
Updates allowlist reference |
lib/sandbox/unittest/CSandboxUserNamespaceProbeTest_Linux.cc |
Validates probe modes |
lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc |
IPC directory tests |
lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc |
Mechanism assertions |
lib/sandbox/unittest/CMakeLists.txt |
Registers sandbox tests and payload |
lib/sandbox/CSandboxedProcessSpawner_Linux.cc |
Creates IPC directories before validation |
lib/sandbox/CPytorchInferenceSandboxPolicy.cc |
Adds mounts, syscall grants, and directory setup |
lib/sandbox/CMakeLists.txt |
Updates sandbox documentation |
lib/core/unittest/CDetachedProcessSpawnerTest.cc |
Environment stripping tests |
lib/core/CDetachedProcessSpawner.cc |
POSIX marker filtering |
lib/core/CDetachedProcessSpawner_Windows.cc |
Windows Unicode environment filtering |
include/seccomp/CSystemCallFilter.h |
Sandbox2-only seccomp startup |
include/seccomp/CMlLegacyBpfSyscallAllowlist.h |
Sandbox2 syscall declarations |
include/sandbox/CPytorchInferenceSandboxPolicy.h |
IPC directory API and contract |
include/core/CDetachedProcessSpawner.h |
Environment helper declarations |
docs/sandbox2_production_failure_modes.md |
Operational signal documentation |
dev-tools/run_sandbox2_attack_defense.sh |
Harness launcher |
build.gradle |
Publishes protocol metadata |
bin/pytorch_inference/Main.cc |
Routes seccomp installation |
bin/controller/unittest/CProcessSpawnerRouterTest.cc |
Router tests |
bin/controller/unittest/CMakeLists.txt |
Registers router tests |
bin/controller/unittest/CCommandProcessorTest.cc |
Routing token tests |
bin/controller/Main.cc |
Configures sandboxed process paths |
bin/controller/CProcessSpawnerRouter.h |
Router API |
bin/controller/CProcessSpawnerRouter.cc |
Route dispatch and signals |
bin/controller/CMakeLists.txt |
Builds and links router |
bin/controller/CCommandProcessor.h |
Uses router-based spawning |
bin/controller/CCommandProcessor.cc |
Parses and strips routing tokens |
3rd_party/controller-protocol.version |
Sets protocol version 2 |
.buildkite/scripts/steps/run_tests.sh |
Adds architecture-specific probe modes |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (::mkdir(dir.c_str(), 0700) == 0) { | ||
| return true; | ||
| } | ||
| return errno == EEXIST; |
There was a problem hiding this comment.
Fixed in 11dc1407f: makeChildIpcDirectory() now accepts EEXIST only when lstat shows an owner-only mode-0700 directory; a regular file or looser directory yields E_CreationFailed. Tests in CPytorchInferenceSandboxPolicyTest.cc.
| const std::wstring wideProcessPath{CStringUtils::narrowToWide( | ||
| processPathHasExeExt ? processPath : processPath + ".exe")}; | ||
| std::wstring wideCmdLine{CStringUtils::narrowToWide(cmdLine)}; |
There was a problem hiding this comment.
No code change: argv paths are ASCII in practice; the pre-change CreateProcess (ANSI) path was equally lossy for non-ASCII environment values. The CreateProcessW switch was required for correct Unicode lpEnvironment handling when stripping ML_SANDBOXED.
| if (WIFEXITED(status) == 0) { | ||
| // Killed by a signal: not a meaningful staged result, but also not | ||
| // the specific exec-failure signature (126/127) - treat as a | ||
| // staged failure rather than a hard harness-broken failure. | ||
| return EProbeOutcome::E_StagedFailure; |
There was a problem hiding this comment.
Fixed in 11dc1407f: signaled probe payload is E_Crashed and fails the harness in every ML_SANDBOX2_REQUIRE mode.
| if not data: | ||
| break |
There was a problem hiding this comment.
No code change: on Linux, FIFO hang-up is reported only after a writer has connected and left (pipe_poll writer count), so the reader does not exit on an empty read() before the child connects.
valeriy42
changed the title
c7b361b to
0721a82
Compare
The four sandbox_enabled tests were annotated @AwaitsFix pending the paired ml-cpp artifact carrying the /proc mount and controller-protocol version-2 marker (elastic/ml-cpp#3188). With that artifact in place the tests run green, so enable them. Sequencing note: the ml-cpp snapshot resolved by ES CI must contain #3188 before this PR merges.
fb346e0 to
0d366ad
Compare
| if (::mkdir(dir.c_str(), 0700) == 0) { | ||
| return true; | ||
| } | ||
| return errno == EEXIST; |
There was a problem hiding this comment.
There's a mismatch between docs and implementation: the comment above (and the E_CreationFailed enum doc in the header, "a non-directory in the way") says a non-directory occupying dir is reported as a failure, but mkdir on an existing regular file or symlink returns EEXIST, so this returns true (success). Only an ENOTDIR from a non-directory parent component would be caught. It's not exploitable (validateChildIpcLaunchSpec canonicalizes and rejects afterward via E_MutableSymlinkOrAlias/shape checks), but either fix the comment or tighten with an lstat/S_ISDIR check on the EEXIST branch so the code matches its stated contract.
There was a problem hiding this comment.
Same fix as the Copilot thread (11dc1407f).
| // is not logged again here: an empty deployment_id in the signal is | ||
| // itself the observable symptom, and the Sandbox2 spawner (when that | ||
| // route is actually taken) logs the failure with detail. | ||
| ml::sandbox::ensureChildIpcDirectory(trustedTmpDir, args); |
There was a problem hiding this comment.
I can't spot anything in the router or spawner that ever removes $TMPDIR/ml-child-ipc/<child-id>, and this call now creates it on every sandbox-eligible spawn, including the legacy/degraded route, purely so the sandbox2_launch signal has a real deployment_id. So a plain no-token launch of pytorch_inference now leaves a 0700 directory behind even when Sandbox2 is never involved. I'm just wondering what reaps these across the child's lifetime and on controller restart? If Elasticsearch owns cleanup then a brief comment would help.
There was a problem hiding this comment.
Documented on ensureChildIpcDirectory() in 11dc1407f: per-deployment directory, reused across restarts, empty after FIFO unlink. Removal tracked in #3214.
| } else { | ||
| // Not a sandboxed process path: unrelated processes always go via | ||
| // the legacy spawner, unchanged from today's behaviour. | ||
| spawned = m_LegacySpawner.spawn(processPath, args, childPid); |
There was a problem hiding this comment.
If a caller passes route == E_Sandbox2 but processPath is not a configured sandboxed path, this falls through to the legacy spawner and emits no sandbox2_launch signal (sandboxEligible == false). CCommandProcessor makes this unreachable in production (it rejects --requireSandbox on non-sandboxed paths), but this router is a standalone public class whose contract permits the combination. A LOG_WARN/assert on "E_Sandbox2 requested for a non-sandboxed path" would harden it against future callers rather than silently downgrading to legacy.
There was a problem hiding this comment.
Addressed in 11dc1407f by documenting on ERoute::E_Sandbox2 and the router else branch that the enum default is not a routing decision for non-sandboxed paths (so we do not LOG_WARN on every ./autodetect launch).
| // ./pytorch_inference always takes the legacy route (see | ||
| // CCommandProcessor), so listing it here fails nothing on macOS, | ||
| // Windows, or a Linux build without Sandbox2 support. | ||
| ml::controller::CCommandProcessor::TStrVec sandboxedProcessPaths{"./pytorch_inference"}; |
There was a problem hiding this comment.
This comment ("fails nothing on macOS, Windows, or a Linux build without Sandbox2 support") is only true for the no-token path. Now that ./pytorch_inference is listed as being sandboxed on every platform, an explicit --requireSandbox for it on a non-Sandbox2 build routes to E_Sandbox2, then hits the router's #else, and then fails the launch. Which is all arguably correct (since the operator asked for a sandbox the build can't provide), but it's probably worth (a) confirming that ES never sends --requireSandbox on non-Linux, and (b) tightening this comment so it doesn't read as "no token ever fails here."
There was a problem hiding this comment.
Comment rewritten in 11dc1407f: no-token never fails; --requireSandbox without Sandbox2 fails closed on this PR; ES sends tokens only on Linux.
| //! legacyBpfAllowedSyscalls() - tracked so sandbox2AllowsAllLegacySyscalls() can | ||
| //! assert Sandbox2 never grants strictly less than the legacy filter without | ||
| //! requiring every one of these to be repeated in sandbox2ExplicitSyscalls(). | ||
| inline std::vector<int> sandbox2HelperCoveredSyscalls() { |
There was a problem hiding this comment.
sandbox2AllowsAllLegacySyscalls() trusts this hand-maintained list to reflect what the Sandbox2 PolicyBuilder helpers (AllowRead/AllowWrite/...) actually grant. A future sandboxed-api upgrade could change helper coverage, and the assertion would still pass while the real applied policy diverges (granting less than the legacy filter). I think its worth a one-line note here that this list must be re-verified whenever the pinned Sandbox2 version is bumped.
There was a problem hiding this comment.
Removed sandbox2AllowsAllLegacySyscalls() / sandbox2HelperCoveredSyscalls() in 11dc1407f — the policy builder’s legacyBpfAllowedSyscalls() loop is what actually mirrors the legacy filter.
| __NR_getgid, | ||
| __NR_geteuid, | ||
| __NR_getegid, | ||
| ML_NR_statx, |
There was a problem hiding this comment.
This newly-added list (and also sandbox2ExplicitSyscalls() above) now depends on ML_NR_statx/ML_NR_rseq, which are only #defined inside the __x86_64__/__aarch64__ branches near the top of this header, yet are used unconditionally under __linux__. ML_NR_clone3 has a global fallback; these two don't, so a Linux build on any other arch fails with a confusing "undeclared identifier" from right here. Given that ml-cpp only supports x86_64/aarch64 today this is a minor nit, but a #else #error "unsupported architecture" closing the arch block would make the unsupported-arch failure explicit.
There was a problem hiding this comment.
Added #else #error for unsupported Linux arch in 11dc1407f.
| __NR_getpriority, | ||
| __NR_tgkill, | ||
| __NR_statfs, | ||
| __NR_connect, |
There was a problem hiding this comment.
__NR_connect is granted to the Sandbox2 policy here (and also carried in the legacy list), but this policy adds no network namespace, so connect is actually reachable inside the sandbox. Is this because it's needed for the log-pipe/unix socket? Maybe add a justifying trailing comment? Just a one-liner on why connect (and mknod/mknodat) are required would keep the allowlist self-documenting.
There was a problem hiding this comment.
Trailing comments on __NR_mknod* (mkfifo) and __NR_connect (AF_UNIX pipe connect) in sandbox2ExplicitSyscalls() in 11dc1407f.
| // pytorch_inference receives its --input=/--output=/--restore=/--logPipe= | ||
| // argv from Elasticsearch as host paths under this root; a remap would | ||
| // leave those paths unresolvable inside the sandbox's own mount namespace. | ||
| policyBuilder.AddDirectory(validated.s_Spec.s_ChildIpcRoot, /*is_ro=*/false); |
There was a problem hiding this comment.
Nit on the /dev/null//dev/urandom//dev/random loop earlier in this function (inherited from #3187, and unchanged here, so anchored on this line): is_ro = std::strcmp(devFile, "/dev/null") != 0 is correct (only /dev/null is writable, the RNG devices read-only) but took me a while to understand. Again a one-line comment ("only /dev/null needs to be writable") would help readability.
There was a problem hiding this comment.
One-line /dev/null writable-only comment in 11dc1407f.
| // sandbox2_launch signal reports as "route":"sandbox2". | ||
| const bool sandbox2Launched{ml::seccomp::sandbox2LaunchedChild()}; | ||
| const ml::seccomp::SInProcessFilterResult seccompResult{ml::seccomp::applyInProcessSeccompFilter( | ||
| sandbox2Launched, TERMINATE_ON_DEGRADED_SECCOMP_FAILURE, |
There was a problem hiding this comment.
With TERMINATE_ON_DEGRADED_SECCOMP_FAILURE deliberately false, a legacy-route launch whose in-process seccomp install fails (mechanism unavailable) still proceeds to process untrusted model input unsandboxed. This is intentional and very well documented here and in CSystemCallFilter.h, gated on the future guarantee that every launch carries an explicit token. We should make sure there's a tracked follow-up issue for the flip, since this is the one place the design is knowingly open.
edsavage
left a comment
edsavage
left a comment
There was a problem hiding this comment.
LGTM
Just a few non-blocking changes/nits
0d366ad to
5174aa8
Compare
The clean rebuild's Sandbox2 policy builder grants syscalls to pytorch_inference by looping over legacyBpfAllowedSyscalls() alone - the same list used to build the legacy in-process BPF filter. That is not sufficient: Sandbox2's namespace/threading setup makes pytorch_inference exercise syscalls (scheduling, epoll, pipes, directory/file management for forecast temp storage) that the simpler legacy filter never needed a grant for, since it never sets up namespaces or Sandbox2's own thread/monitor machinery. #2873's enhancement/sandbox2 branch already solved this with a dedicated sandbox2ExplicitSyscalls() list, granted in addition to what Sandbox2's PolicyBuilder helpers (AllowRead/AllowWrite/AllowOpen/etc.) cover implicitly. That list was never carried into this rebuild's CPytorchInferenceSyscallAllowlist.h, so any syscall in it (confirmed via a real run: sched_getaffinity) got denied and killed the sandboxed process. Ports sandbox2ExplicitSyscalls() and sandbox2HelperCoveredSyscalls() from the original branch, wires the explicit list into buildPytorchInferencePolicy() alongside the existing legacy-list loop, and adds sandbox2AllowsAllLegacySyscalls() plus a regression test so a future change to legacyBpfAllowedSyscalls() without a corresponding Sandbox2 grant fails loudly instead of silently regressing.
…box2 buildPytorchInferenceFilesystemPolicy() mounted spec.s_ChildIpcRoot into the sandbox at a fixed remapped path (/run/elastic/ml-ipc) instead of its own host path. Elasticsearch constructs pytorch_inference's --input=/--output=/--restore=/--logPipe= argv using the real host path under $TMPDIR/ml-child-ipc/<child-id>, so once Sandbox2 enforcement actually engaged (after the previous two fixes), pytorch_inference could not find its own pipes: it looked for them at the host path baked into its argv, which does not exist inside its own mount namespace once mounted under a different name. Mounts the child IPC root at the same absolute path inside and outside the sandbox instead, matching the convention every other mount in this policy already uses (AddDirectory, not AddDirectoryAt). Updates the doc comments in CPytorchInferenceSandboxPolicy.h and build.gradle's controller-protocol contract comment that described the old remap, and updates CPytorchInferenceSandboxPolicyMechanismTest_Linux's probe invocation to pass the host-visible childRoot instead of the now-removed hardcoded /run/elastic/ml-ipc literal. Verified on the devbox: CPytorchInferenceSandboxPolicyMechanismTest_Linux passes (ipc_readwrite: allowed), and PyTorchSandboxIT#testConcurrentDeploymentsDoNotCollideUnderIsolatedChildIpcDir passes end-to-end against a rebuilt ml-cpp artifact (0 failures, 0 errors).
Sandbox2 mounts a fresh, PID-namespaced procfs at /proc on the outer root before it builds and pivots into the sandbox chroot, then detaches the old root - so the pivoted rootfs has no /proc unless the policy adds one. The E_MountNamespacedProcfs case was a no-op on the false assumption that Sandbox2 provides /proc inside the rootfs automatically. With /proc absent, readlink(/proc/self/exe) and open(/proc/self/maps) both fail with ENOENT. Intel oneMKL's runtime dispatcher reads /proc/self/exe to self-locate and dlopen its CPU-specific libmkl_*.so.3 kernels; the failed read makes it abort with "Intel oneMKL FATAL ERROR: Cannot load <mkl-loader>", killing every sandboxed pytorch_inference under enforced Sandbox2. Bind /proc into the rootfs. Because Sandbox2 mounts the fresh procfs after CLONE_NEWPID and before PrepareChroot, the bind captures that already namespaced procfs (never the host's): verified inside the sandbox, /proc shows only the sandboxee's own PIDs (2) versus 194 on the host. /sys is left unmounted (E_Skip) - no fresh namespaced /sys exists to bind outside a network namespace, and pytorch_inference/libtorch run without it. Verified on the qaf local-deployment harness with xpack.ml.trained_models.sandbox_enabled=true (mode:enforced): the trained-model boot check and 7/8 test_scenario_buildly inference scenarios pass with zero MKL crashes across all enforced launches.
The Sandbox2 filesystem policy had no test exercising /proc, so the missing /proc mount that broke Intel oneMKL's runtime dispatcher went undetected. Add a proc_self_exe mechanism to ml_sandbox_probe (readlink /proc/self/exe inside the real sandbox) and assert it in the policy mechanism test. Without the /proc mount this readlink fails with ENOENT and the check reports "unreadable", turning the "Cannot load <mkl-loader>" crash into a build-time test failure.
4d05aad to
0d432b1
Compare
install_libs() guarded its per-library copy loop with a single coarse glob: "did the source directory contain anything at all matching *<prefix>*<postfix>?". Any unrelated library sharing that prefix and suffix satisfied the guard, after which every individual library that did not match was skipped silently and the build still reported success. On a toolchain whose Intel MKL SONAMEs have moved to .so.3, the unrelated libmkl_scalapack_*.so.2 / libmkl_cdft_core.so.2 cluster libraries kept the guard happy while all twelve requested MKL libraries were skipped, producing a distribution in which pytorch_inference cannot resolve libmkl_intel_lp64.so.3, libmkl_gnu_thread.so.3 or libmkl_core.so.3. The dynamic loader then kills the process with exit code 127 before it has reconfigured logging, so there is no log pipe and no diagnostic anywhere - Elasticsearch only sees the named pipe never appear. Check each requested library individually and fail the build with the name of the pattern that did not match, so a toolchain change can never again silently produce an unrunnable distribution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Set the RPATH of every bundled 3rd-party library to $ORIGIN unconditionally, rather than only overwriting libraries that already declared one. Some prebuilt libraries (notably Boost) ship with no RPATH at all, and because DT_RUNPATH is not inherited transitively a library with a sibling dependency (e.g. libboost_log -> libboost_atomic) failed to load at runtime even though the dependency sits right beside it. Elasticsearch's Spawner clears the native controller's environment, so RPATH is the sole resolution mechanism - there is no LD_LIBRARY_PATH fallback. Also skip non-ELF files via patchelf exit status. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed RunAsync() discarded the Result returned by AwaitResult() and logged only "Sandbox2 failed to start <path>". This is a fail-closed path with no legacy fallback, so the deployment start fails outright and the router's sandbox2_launch signal can only report mode="fail_closed" - it has no field for a cause. The sandboxee's own stderr dies with the sandboxee, so an operator was left with a launch that failed for no stated reason. Log the Result's status and reason code instead, which distinguishes (for example) a SETUP_ERROR/FAILED_SUBPROCESS - the forkserver could not create its initial namespaces, typical of a container runtime whose seccomp policy blocks clone(CLONE_NEWUSER) - from a policy VIOLATION or an internal error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A host that denies any one of Sandbox2's forkserver startup prerequisites fails every --requireSandbox launch with the same opaque SETUP_ERROR/FAILED_SUBPROCESS result. The real reason is written to the sandboxee's stderr, which dies with the sandboxee; the controller's own stderr is redirected onto the ML log pipe, where Elasticsearch discards anything not framed as a JSON log message (CppLogMessageHandler stores it as a fatal-error string that is only surfaced when the process dies, and the controller does not die). So the cause was unrecoverable from ES logs. Restore CSandbox2Diagnostics, dropped when ml-cpp#2873 was recreated as the PR A-E stack, and make it active rather than passive. The original read kernel.unprivileged_userns_clone and user.max_user_namespaces; both are host-global and are inherited unchanged by a container whose seccomp or LSM policy denies the operation anyway, so on their own they report a healthy environment on exactly the hosts where the sandbox cannot start. The probe instead performs the forkserver's own sequence in throwaway children - unshare(CLONE_NEWUSER), id-map writes, unshare(CLONE_NEWNS|CLONE_NEWPID), fork, mount(tmpfs), mount(procfs) - and names the step that was denied. Verified against four container configurations whose behaviour was established independently: default Docker reports E_UserNamespaceDenied, seccomp=unconfined reports E_TmpfsMountDenied, seccomp+apparmor=unconfined reports E_ProcMountDenied, and all three unconfined reports E_Available. The tmpfs case is the one where a real launch deadlocks rather than failing, so the probe is also the only signal available for it. Logged once at controller start, not lazily on first launch: an operator deciding whether to enable xpack.ml.trained_models.sandbox_enabled needs the answer before a deployment fails closed. Runs entirely in forked children, so the controller's own namespaces and mount table are never touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
testSandbox2ExplicitSyscallsCarriedForwardFromPr2873 did not compile: it
called ml::seccomp::pytorch_inference::sandbox2AllowsAllLegacySyscalls(),
but the function lives in ml::seccomp - a leftover from the ml-cpp#2873
layout, where it sat in a pytorch_inference sub-namespace. So the whole
ml_test_seccomp target failed to build on this branch.
Fixing the qualifier exposed a second, latent bug in the same test. It
built its set as
std::set<int>{sandbox2ExplicitSyscalls().begin(),
sandbox2ExplicitSyscalls().end()}
and sandbox2ExplicitSyscalls() returns std::vector<int> by value, so
begin() and end() came from two different temporaries. Walking from one
buffer to the end of another is undefined behaviour: the test passed when
run on its own and segfaulted inside the full suite, depending only on
heap layout. Call it once and build the set from that vector.
Verified: the full ml_test_seccomp suite now passes on five consecutive
runs, where it previously segfaulted in this test case.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lity
The controller marks itself non-dumpable (PR_SET_DUMPABLE=0) during
startup, and probeSandbox2Capability()'s forked child inherited that. A
non-dumpable process's /proc/self files are owned by root, so the child's
open("/proc/self/uid_map") failed with EACCES and the probe reported
E_IdMapWriteDenied on a host that fully supports Sandbox2.
It went unnoticed because the one production caller, the startup
self-check, happens to run just before makeProcessNonDumpable(). Any
caller after that point - for example a routing decision taken at spawn
time - got the wrong answer, and the logged self-check and the decision
would silently disagree. Sandbox2 itself is unaffected: its forkserver is
freshly exec()ed, which resets dumpability.
Restore dumpability in the throwaway probe child, matching the state the
real forkserver starts in. The caller's own dumpability is untouched.
The new regression test runs the probe from a non-dumpable child and
requires the same verdict as from a dumpable caller. Verified it fails
without this change (E_IdMapWriteDenied vs E_Available) and passes with
it; the full ml_test_sandbox suite passes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tighten ensureChildIpcDirectory EEXIST handling, document router/ERoute semantics, remove the non-enforcing sandbox2AllowsAllLegacySyscalls check, harden the userns probe harness against signal deaths, and correct the sandbox2_launch route/mode documentation ahead of splitting Landlock work.
98d6074 to
9eadbdd
Compare
Setting $ORIGIN on every bundled library (085b8de) also rewrote the Intel MKL libraries, which previously shipped untouched. On glibc 2.34 (Amazon Linux 2023, the Java Inference Integration Test agents) pytorch_inference then dies with SIGSEGV during the first inference of ELSER and E5, shortly after installing its seccomp filter, and Elasticsearch reports "inference canceled as process is stopping". Tiny test models and newer glibc are unaffected, which is why the multi-node tests and local runs still passed. Reproduced with the CI build image and an AL2023 container: the same binaries fail with the MKL libraries patched and pass with only those libraries restored. MKL does not need the RPATH: libtorch_cpu (already $ORIGIN) pulls in the MKL entry points, and the kernels MKL dlopen()s only NEED libmkl_core, which is already loaded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
## Summary Stacks on #3188. When Elasticsearch sends `--requireSandbox` but the host cannot run Sandbox2 (ECH allocators with user namespaces disabled, container seccomp blocking `CLONE_NEWUSER`, and similar), the controller steps down to a Landlock filesystem ruleset plus the existing in-process seccomp filter instead of failing every deployment closed. - `hostConfinement()` probes once at controller start (same cached verdict as the startup self-check) and walks a fixed ladder: Sandbox2, then Landlock, then refusal with an operator-actionable message returned to Elasticsearch. - New `sandbox2_launch` mode `"landlock"` (`route:"sandbox2"`, `sandbox2_established:false`). Consumers must not treat `route` alone as full isolation. - Router-only `--restrictFilesystem` token; `CCommandProcessor` rejects caller-supplied copies. - Documented in `docs/sandbox2_production_failure_modes.md`. Companion: elastic/elasticsearch#159052 must accept `mode:"landlock"`. A failed Sandbox2 launch on a host that can run Sandbox2 is never retried under Landlock.
## Summary Adds `xpack.ml.trained_models.sandbox_enabled` (operator-dynamic, node-scoped, default `false`) to control whether `pytorch_inference` runs under Sandbox2 or the legacy `posix_spawn` + in-process-seccomp path, plus the isolated per-child IPC layout and validation Sandbox2 needs. - `false` (default): `PyTorchBuilder` sends `--disableSandbox` on Linux; the controller uses the legacy spawner. Dark-launched for 9.6 — `true` is not yet supported for on-prem deployments. - `true`: sends `--requireSandbox` on Linux instead, so the controller must use Sandbox2, with no automatic fallback to legacy on a failed spawn attempt. Non-Linux nodes send neither token either way (Sandbox2 is Linux-only). - Both are explicit, per-launch tokens on the controller command channel, never omitted and never inferred from a controller-side default. A dynamic setting change only affects deployments started after the change; already-running deployments keep their original launch mode. ## Isolated per-child IPC directory (Linux, `sandbox_enabled=true`) `NamedPipeHelper`/`ProcessPipes` gained a second pipe-naming layout: when isolation is requested, `input`/`output`/`restore`/`logPipe` all live under `$TMPDIR/ml-child-ipc/<deployment_id>/` instead of the legacy flat, pid-suffixed names. Elasticsearch only constructs the path strings; the native controller creates and owns the `0700` directory. Autodetect/DFA/normalizer callers are unaffected and keep the legacy layout. ## `deployment_id` validation `StartTrainedModelDeploymentAction.Request#validate()` rejects a `deployment_id` that isn't safe as a single path component (empty, `.`, `..`, or containing `/`, `\`, or NUL) since it shapes the isolated IPC path. Deliberately narrower than the ML plugin's general `isValidId` charset check, so existing (possibly mixed-case) inference-endpoint ids used verbatim as `deployment_id` keep working. `NamedPipeHelper#getChildIpcDirectoryPrefix` re-checks the same property as defense-in-depth at the point the path is actually built. ## Cross-repo dependency (must merge as a coordinated pair) Companion: elastic/ml-cpp#3188. That PR defines the `--disableSandbox`/`--requireSandbox` controller wire protocol and bumps `controller-protocol.version` to 2. `x-pack/plugin/ml/build.gradle`'s `verifyControllerProtocolVersion` task requires >= 2 and hard-fails the build if the resolved ml-cpp bundle doesn't publish it — do not merge this ahead of the ml-cpp dependency bump. ## Tests - `PyTorchBuilderTests`: `--disableSandbox` / `--requireSandbox` emitted correctly per setting value, on Linux only. - `ProcessPipesTests`, `NamedPipeHelperTests`: legacy naming unchanged byte-for-byte; isolated naming construction; childId safety guard. - `NativePyTorchProcessFactoryTests`: the `sandboxEnabled` snapshot is shared between the isolation-flag and command-line-token decisions within one launch. - `StartTrainedModelDeploymentRequestTests`: `deployment_id` path-safety, including packaged/default model ids and `_SNAPSHOT` suffixes. - `PyTorchSandboxIT`: default/explicit-setting deployment start and inference, concurrent-deployment isolation under legacy naming. The full enforced-sandbox case is `@AwaitsFix` pending the ml-cpp artifact. ## Notes for reviewers - Superseding #154825, which carried approvals against an earlier design iteration (default `true`, ml-cpp#2873, no IPC isolation, single-token protocol). Re-review requested. - Labels to apply: `:ml`, `>enhancement`, `v9.6.0`. - Surfacing/countering ml-cpp's structured enforced-mode signal (deployment_id/model_id/route/mode) is tracked as a separate follow-up, not implemented here. ### Companion PRs - elastic/ml-cpp#3188 — defines the two-token controller protocol this PR sends
Summary
Stacks on #3187. Adds typed controller-side routing behind two symmetric controller tokens. Landlock fallback when Sandbox2 is unavailable is split to stacked #3215.
CCommandProcessorparses at most one of--disableSandbox(operator kill switch, forces legacy) or--requireSandbox(operator opt-in, forces Sandbox2 on capable hosts) for the exact configuredpytorch_inferencepath. Duplicate occurrences of either token, a mismatched process path, or both tokens present together are all rejected before spawn. The selected token is stripped before the child ever sees it.CProcessSpawnerRouterdispatches the decided route toCSandboxedProcessSpawnerorCDetachedProcessSpawnerwith no automatic fallback: a required Sandbox2 launch that fails returns a failed start response, it never retries through the legacy spawner.startcommand with neither token always takes the legacy route - permanent behaviour for any caller that sends no routing token (support/debug scripts, direct controller invocation, the test harness), not a rollout seam. Elasticsearch is expected to always send exactly one of the two tokens per launch, chosen from its own operator setting's live value.sandbox2_launch:deployment_id,model_id,route,sandbox2_established,mode,legacy_reason,sandbox2_compiled_in) so a consumer can distinguish an operator kill switch from the no-token default, and "Sandbox2 supported but no routing token sent" from "built without Sandbox2 support."ML_SANDBOXEDis stripped from every legacy-route child's environment (POSIX and Windows) so an inherited or injected value can never fail-open the mandatory in-process seccomp filter.CSandbox2Diagnostics) so operators see whether--requireSandboxcan succeed before a deployment fails closed.EPERMonmount("proc", ...)and there is currently no userns-capable x86_64 CI runner.test_sandbox2_attack_defense.py: reached markers, unsandboxed positive controls, per-case cleanup/reap assertions, the realml-child-ipc/<child-id>IPC layout, an assertion that the controller's ownsandbox2_launchsignal reports the expected route before any security-boundary check runs, and explicit--requireSandbox/--disableSandboxtokens per case instead of a global env-var default.3rd_party/controller-protocol.version, now version 2) for a future cross-repo compatibility check.install_libs()guard and unconditional$ORIGINRPATH on bundled ELF libraries so sibling.sodependencies resolve when the controller environment is cleared.Filesystem-policy fixes found during enforced-mode qualification
End-to-end qualification on the qaf harness with
xpack.ml.trained_models.sandbox_enabled=true(realmode:enforcedroute, not the legacy fallback) surfaced several launch/filesystem-policy gaps that previously only manifested once a realpytorch_inferenceran under the enforced sandbox. Fixed here:$TMPDIR/ml-child-ipc/<child-id>(mode 0700) beforevalidateChildIpcLaunchSpec()'s liverealpath()calls, on both spawn paths. Elasticsearch only ever constructs the IPC path strings; nothing created the directory they name./run/elastic/ml-ipcremap), becausepytorch_inferencereceives its--input=/--output=/--restore=/--logPipe=argv as host paths under that root - a remap left them unresolvable in the sandbox mount namespace.legacyBpfAllowedSyscalls()alone is insufficient./procis mounted (PID-namespaced) inside the sandbox rootfs. Sandbox2 mounts a fresh PID-namespaced procfs on the outer root, thenpivot_roots into the chroot and detaches the old root, so the rootfs had no/proc. Without itreadlink(/proc/self/exe)fails withENOENT, breaking Intel oneMKL's runtime dispatcher (it reads/proc/self/exeto self-locate anddlopenits CPU-specificlibmkl_*.so.3kernels) - every enforcedpytorch_inferenceaborted withIntel oneMKL FATAL ERROR: Cannot load <mkl-loader>. The mount binds the already-namespaced procfs (never the host's): inside the sandbox/procshows only the sandboxee's own PIDs./sysis left unmounted.ml_sandbox_probenow checks/proc/self/exeis readable inside the real sandbox, asserted byCPytorchInferenceSandboxPolicyMechanismTest_Linux- turning the MKL crash into a build-time failure.Verified: ml-cpp sandbox unit tests 33/33; qaf boot check and
test_scenario_buildly(8/8) pass undermode:enforcedwith zero MKL crashes and no legacy fallbacks.