[ML] Add Sandbox2 security integration for PyTorch inference#2873
Open
valeriy42 wants to merge 44 commits into
Open
[ML] Add Sandbox2 security integration for PyTorch inference#2873valeriy42 wants to merge 44 commits into
valeriy42 wants to merge 44 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
valeriy42
changed the title
valeriy42
marked this pull request as draft
October 28, 2025 14:00
- Added new tests for Sandbox2 functionality, including privilege validation, filesystem isolation, and syscall filtering. - Introduced a TestCleanup class for managing temporary files during tests. - Updated the CDetachedProcessSpawner_Linux.cc to support new command line arguments for log handling and model path. - Renamed modelDir to modelPath for clarity and adjusted policy building to accommodate file access. - Improved overall test coverage for Sandbox2 features and ensured graceful degradation when Sandbox2 is not available.
- Improved error handling for cases when Sandbox2 is disabled or unavailable for pytorch_inference processes. - Enhanced logging to provide clearer feedback on spawning failures with Sandbox2. - Updated comments for clarity regarding the fallback to standard posix_spawn for non-pytorch_inference processes.
- Added a new function to apply standard ML syscall restrictions using Sandbox2's PolicyBuilder, ensuring consistent security across ML processes. - Updated the CDetachedProcessSpawner_Linux to utilize the new syscall policy for pytorch_inference, eliminating the need for seccomp filtering in this context. - Enhanced comments and documentation to outline future migration plans for other ML processes to Sandbox2. - Noted the gradual transition from seccomp filters to Sandbox2 policies in the CSystemCallFilter_Linux implementation.
- Removed SetUserAndGroup from PolicyBuilder due to updates in the sandboxed-api. - Updated AddTmpfs to include a size parameter for better resource management. - Refactored Sandbox2 instantiation to use unique_ptr for the executor, improving memory management. - Enhanced comments to clarify changes and provide context for future updates.
- Removed outdated syscall number definitions and replaced them with fallback definitions for newer syscalls, ensuring compatibility with RHEL8 headers. - Updated the handling of input pipes in the sandbox policy to allow read and write access, improving functionality. - Enhanced comments for better clarity on syscall handling and future maintenance.
…l-cpp into enhancement/sandbox2
…for Linux - Deleted Sandbox2SecurityTest.cc as it is no longer needed. - Updated CMakeLists.txt to remove references to the deleted test file. - Introduced CDetachedProcessSpawnerTest_Linux.cc, which includes tests for process spawning and integration with Sandbox2. - Enhanced CMakeLists.txt to link against Sandbox2 libraries for the new tests.
valeriy42
marked this pull request as ready for review
July 17, 2026 16:21
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @valeriy42, I've created a changelog YAML for you. |
CI configures the build with -DCMAKE_UNITY_BUILD=ON, which propagated into the FetchContent'd Abseil and Sandboxed API sources. Abseil's absl_time_zone target is not unity-build safe: kDigits is defined in an anonymous namespace in both time_zone_fixed.cc and time_zone_posix.cc, and these collide when merged into a single unity translation unit, failing the x86_64 RelWithDebInfo (and debug) builds early in compilation. Disable CMAKE_UNITY_BUILD around the third-party FetchContent block and restore the caller's setting afterwards. ml-cpp's own sources remain unity-built.
pytorch_inference runs inside a Sandbox2 mount namespace and creates its named pipes with mkfifo(). On the x86_64 CI agents, whose build paths are long enough to trigger the TMPDIR override, the log FIFO never became visible at the host path Elasticsearch waits on, so the connection timed out with FileNotFoundException while the sandboxee blocked in open(O_WRONLY). - Restore the controller's global TMPDIR on every exit path of the sandboxed spawn. Previously the override to /tmp (needed to keep the Sandbox2 forkserver socket under the 108-char limit) was never undone, so the second and subsequent spawns saw the short value, skipped the per-process TMPDIR restoration and launched pytorch_inference with the wrong TMPDIR. - Bind-mount both the canonical (realpath) and the literal pipe directory when they differ, so a mkfifo() against the literal path the sandboxee uses is always backed by the host directory rather than the sandbox throw-away rootfs. - Add a host-side diagnostic that reports whether/when the log FIFO appears, to distinguish mount-visibility from start-up latency in CI.
…ailure The sandboxed pytorch_inference log FIFO never becomes visible on the host directory Elasticsearch watches on CI (both the ml multi-node and inference service suites), so ES fails to connect and reports FileNotFoundException. The 30s appearance poll confirms this is a hard mount-visibility problem, not start-up latency. The controller cannot currently observe why: Sandbox2 owns and reaps the sandboxee so the waitpid tracker never sees it, and neither the Sandbox2 forkserver/monitor stderr (mount warnings, seccomp violations) nor the sandboxee's own early stderr is captured by Elasticsearch. Add temporary diagnostics, surfaced through the ml-cpp logger so they reach the ES node log: * capture the Sandbox2 forkserver/monitor stderr by redirecting the controller's fd 2 to a file before the global forkserver is started; * capture the sandboxee stdout/stderr via Executor::ipc()->MapFd(); * dump both from the existing log-FIFO appearance diagnostic thread. To be reverted once the root cause is identified.
The previous diagnostic commit redirected the controller's fd 2 to a file to capture the Sandbox2 forkserver/monitor stderr. On CI the controller's log stream is read from its stderr, so this silenced ALL controller logging after the redirect (build 2783 shows controller output stopping right after the "TMPDIR path too long" message), including the diagnostic dumps themselves. Remove the fd 2 redirect and the forkserver-stderr capture. Keep the safe sandboxee capture: map pytorch_inference's stdout/stderr with Executor::ipc()->MapFd() and dump them via the ml-cpp logger from the diagnostic thread, which will surface the mkfifo()/open() errno that explains why the log FIFO never appears on the host. Still diagnostic-only, to be reverted once the root cause is identified.
The sandboxed pytorch_inference produces no stdout/stderr on CI and its log FIFO never appears on the host, but the controller's tracker never reaps it (it is a child of the Sandbox2 forkserver), so we had no signal for how it actually terminates. Move the sandbox instance into the diagnostic thread so it owns the lifetime, wait on AwaitResult, and log the final status (SETUP_ERROR / VIOLATION / SIGNALED / exit code) and reason code alongside the captured sandboxee output. This distinguishes a namespace/mount setup failure from a seccomp violation or a silent block on the log pipe.
…dbox2 debugging Short-circuit run_es_tests.sh to run only the native-multi-node :x-pack:plugin:ml:qa:native-multi-node-tests:javaRestTest task filtered to *PyTorchModelIT. The Sandbox2 pytorch_inference FIFO-visibility failure is universal and fires on the first spawn, so a single deployment test class reproduces it in minutes instead of running the full ~40+ min suite. REVERT before merge.
Override the generated PR pipeline (regardless of labels/comments) to a single Linux x86_64 build plus the multi-node ML integration tests only. Disables the aarch64, macOS and Windows builds and their ES-test steps, and the secondary ES inference-API test step, so each debugging round-trip is one build + one pytorch test class instead of five platform builds and the full ES suites. REVERT before merge.
The AwaitResult diagnostic revealed that the sandboxed pytorch_inference is killed by a seccomp violation on clone3 (syscall 435) the moment libtorch spawns a thread, before it can create its log FIFO - which is why the FIFO never appeared on the host and the sandboxee produced no output. The policy did guard clone3 with '#ifdef __NR_clone3', but the CentOS 7 based CI build image has kernel headers that predate clone3, so __NR_clone3 is undefined there and the rule was compiled out, while the newer glibc on the CI runtime uses clone3 for thread creation. aarch64 was unaffected because its glibc still uses clone. Allow clone3 by its literal number (435 on both x86_64 and aarch64) so the policy no longer depends on the build image's header version.
Now that the root cause (clone3 seccomp violation) is fixed and CI is green, remove the temporary diagnostics and debugging shortcuts: - CDetachedProcessSpawner_Linux.cc: drop the FIFO-appearance poll, the Executor MapFd stdout/stderr capture, logFileTail and the g_DiagSeq counter. Replace the diagnostic thread and the unused g_SandboxMap with a slim monitor thread that owns each sandbox instance for the lifetime of pytorch_inference and logs only genuinely abnormal termination (seccomp VIOLATION or sandbox SETUP_ERROR); normal SIGTERM shutdown is no longer reported as an error. - .buildkite/pipeline.json.py, dev-tools/run_es_tests.sh, build.gradle: revert the temporary single-x86_64 pipeline, the *PyTorchModelIT test narrowing and the strip-skipping so the full build/test matrix runs. - CLogger.cc: revert the stderr log-pipe fallback back to main. - CIoManager.cc: keep the per-stream open-failure error messages but drop the verbose step-by-step debug narration. - Remove the local controller-only ingest reimplementation test; keep the Sandbox2 attack-defense security test and its model generator.
…gging Enforce the exact permitted-path allowlist before Sandbox2 dispatch so a path merely containing pytorch_inference cannot bypass authorization. Add CTrackerThread::removePid and call it from the sandbox monitor thread after AwaitResult so sandboxed PIDs do not leak in the live-process tracker. Mirror checkForDeadChildren termination logging for sandboxed pytorch_inference, including SIGKILL/OOM diagnostics, while keeping normal SIGTERM at INFO.
Contributor
Author
|
buildkite run_serverless_tests |
Contributor
Author
|
buildkite run_qa_tests |
valeriy42
removed
ci:run-qa-tests
|
💚 CLA has been signed |
Contributor
Author
|
buildkite run_qa_tests |
Broaden the Sandbox2 futex policy beyond WAIT/WAKE so timed condition variable waits and requeue paths used under sustained inference load are not SIGSYS-killed. Also raise rlimit_nofile above Sandbox2's default.
…inference daemon QA reproduction (appex-qa build 883) captured the real Sandbox2 diagnostic that earlier CI runs lacked: pytorch_inference was being killed with 'Process TIMEOUT' by Sandbox2's default 120s wall-time limit (and 1024s CPU-time limit), which are designed for run-to-completion sandboxees, not a daemon that stays up for the lifetime of a deployed model. This surfaces in Elasticsearch as 'inference native process died unexpectedly ... Unexpected end of file'. Disarm both limits for pytorch_inference. Also, while investigating: - Rework checkForDeadChildren() in both CDetachedProcessSpawner and its Linux/Sandbox2 specialisation to waitpid() only tracked PIDs individually, rather than waitpid(-1, ...), avoiding interference with the async AwaitResult monitor thread used for sandboxed pytorch_inference. - Restrict /dev mounts in the Sandbox2 policy to the specific device nodes needed (null/urandom/random) instead of the whole directory. - Gate --skipModelValidation behind a new ML_ALLOW_SKIP_MODEL_VALIDATION build option (default OFF), so production/distributed builds cannot disable model graph validation. - Make Sandbox2 a hard requirement on Linux (3rd_party/CMakeLists.txt) instead of silently disabling it if unavailable. - Replace the placeholder Sandbox2Test suite (which only checked host filesystem permissions) with tests that actually spawn pytorch_inference under Sandbox2 and confirm it starts, runs, and terminates cleanly. - Run the Sandbox2 attack-defense integration test from the Docker test entrypoint on Linux.
valeriy42
force-pushed
the
enhancement/sandbox2
branch
from
July 21, 2026 10:34
714f36a to
0d89fc4
Compare
valeriy42
force-pushed
the
enhancement/sandbox2
branch
from
July 21, 2026 11:11
af53f6c to
facd362
Compare
Contributor
Author
|
buildkite run_qa_tests QAF_TESTS_TO_RUN=pytorch_tests |
Contributor
Author
|
buildkite run_qa_tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR introduces Sandbox2 security integration to enhance the security posture of the PyTorch inference process in the ML-CPP codebase.
Changes
Security Enhancements
Build System Updates
Key Components
Security Benefits
Testing
Dependencies
This enhancement significantly improves the security posture of the ML-CPP system by ensuring that PyTorch inference processes cannot compromise the host system.