[ML] Add Sandbox2 security integration for PyTorch inference - #2873
[ML] Add Sandbox2 security integration for PyTorch inference#2873valeriy42 wants to merge 56 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
- 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.
|
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.
714f36a to
0d89fc4
Compare
af53f6c to
facd362
Compare
|
buildkite run_qa_tests QAF_TESTS_TO_RUN=pytorch_tests |
|
buildkite run_qa_tests |
Keep sandboxed pytorch_inference PIDs killable by tracking them outside the waitpid reaper, gate in-process seccomp on an ML_SANDBOXED runtime marker, and fix the attack-defense restore wire format plus leftover debug scaffolding.
0b7aec4 to
39940cb
Compare
Keep the harness as a manual local smoke test; CI coverage remains in CModelGraphValidatorTest and CDetachedProcessSpawnerTest_Linux.
|
buildkite run_pytorch_tests |
Extract the pytorch_inference Sandbox2 policy builder, serialize TMPDIR env mutation, filter ML_SANDBOXED from non-sandbox spawns, and improve logging, tests, and maintainer comments for seccomp BPF offsets.
Add INFO-level spawn-context diagnostics, environment self-checks, and sandbox2::Result capture so deployment failures are triageable from ES logs alone, and propagate spawn failure reasons back to the controller.
Consume the operator kill-switch flag in macOS and Windows spawners so it never reaches pytorch_inference, which does not register it.
…t pairing CCommandProcessor now appends the spawner's failure reason to the START response, so update the two CCommandProcessorTest expectations. The non-existent process case asserts only the stable prefix because the exec error text differs between POSIX (strerror) and Windows (CWindowsError). Move CPytorchInferenceSyscallAllowlistTest out of the seccomp test executable. CSystemCallFilterTest installs an irreversible seccomp filter, and the parallel test runner packs two test cases per process, so any suite scheduled after it dies with EPERM in Boost's per-test-case sigaltstack setup. Also hoist sys/syscall.h out of the namespace in the allowlist header.
|
buildkite run_pytorch_tests |
|
Hi Valeriy, here's my high level first pass. As you suggested I'm starting the review with Layering -
|
There was a problem hiding this comment.
Continuing on from the CDetachedProcessSpawner_Linux.cc notes. I'm reviewing the rest of the changes in the recommended review order (baseline -> seccomp -> pytorch -> controller/IO -> build > tests > docs). Tip reviewed: 6fcdecd30.
CDetachedProcessSpawner.cc (non-Linux)
The --disableSandbox stripping and the failureReason overload look good. There is one gap vs Linux: failureReason is set for allowlist/access failures, but not for setupFileActions / posix_spawnattr_init / posix_spawn failures (those only LOG_ERROR). So the controller START responses on macOS will be less informative for those cases.
Seccomp + CPytorchInferenceSyscallAllowlist.h
The BPF jump updates for __NR_dup look consistent, and sandbox2AllowsAllLegacySyscalls() is a nice drift guard. One clarification: the check asserts Sandbox2 is a superset of the legacy BPF, not full equality (Sandbox2 also allows epoll/renameat/prlimit64/etc.). The “keep in sync” wording could say that explicitly so future editors don’t assume a 1:1 list.
pytorch_inference Main + cmdline
- Seccomp is skipped only when sandboxed. The kill-switch / non-Linux paths still install the filter - this looks correct.
- Please compare
ML_SANDBOXEDto"1"rather thangetenv(...) != nullptr, so that a strayML_SANDBOXED=0can’t disable both Sandbox2 and seccomp. The controller already sets=1. - The argv[0]-is-option workaround in
CCmdLineParseris fragile. I'd prefer fixing the Executor argv construction (and documenting the Sandbox2 quirk) so this special case can go away. As a related note - inside the sandbox spawn branch,fullArgsis built fromargsrather thaneffectiveArgs. These are currently equivalent, buteffectiveArgswould be clearer. SANDBOX2_DISABLEDis referenced inMain.ccguards but I don’t see it defined anywhere in the repo. Is this an intentional leftover escape hatch, or dead?
It's probably also worth flagging for awareness (related to #3098) the gating of --skipModelValidation behind ML_ALLOW_SKIP_MODEL_VALIDATION (default OFF) means distributed builds won’t recognise the flag ES may still send when graph validation is disabled. It's worth confirming the intended prod behaviour with the ES companion PRs before merge.
Controller + CIoManager
The propagation of spawn failure reasons into START responses is a clear ops win, and the per-pipe CIoManager errors will help with FIFO/mount-namespace failures. The propertiesFile on the controller looks orthogonal to sandboxing - I couldn't quite see why it's necessary - a one-line note on why it’s in this PR would help. The ${SANDBOX2_LIBRARIES} on the controller stays empty on the non-Linux path looks right to me.
Build wiring
The Linux FATAL_ERROR if Sandbox2 isn’t built / unity disabled around Abseil/SAPI / licenses/pins look good.
I'd suggest to save/restore BUILD_SHARED_LIBS in 3rd_party/CMakeLists.txt the same way as BUILD_TESTING / CMAKE_UNITY_BUILD - it’s currently forced OFF and left that way for the rest of configure. Configure-time string(REPLACE) / regex patches to SAPI CMake are acceptable while the GIT_TAG is pinned; a short comment block (or *.patch files) listing “required patches + why” would make the next bump safer.
Tests
As far as I can tell CDetachedProcessSpawnerTest_Linux.cc coverage looks solid (allowlist, substring bypass, kill-switch symlink, sandboxed start). The attack-defense harness is explicitly manual / not in CI - can you please confirm it was run on this tip, and consider a slim CI smoke test (benign start + one denied write) later if feasible. Leaving policy-violation coverage only in a manual harness is the main residual test gap.
Docs / cross-cutting questions
- The changelog summary is fine. I'd consider mentioning Linux-only + the
sandbox_enabled/--disableSandboxkill switch for support. - Networking: policy allows
__NR_connectwithout an explicit network-allow helper. Is the Sandbox2 netns empty by default so IP egress is still blocked? If networking isn’t needed, droppingconnect(and documenting netns isolation) would shrink the surface. - FS mounts: RO
/etc+/sysplus RW parent dirs from absolutekey=/pathargs: Can you please confirm why/etc//sysare required, and that ES only passes dedicated pipe dirs into those args.
These are all minor request / nits. My main concerns are the layering / size work from the earlier comment.
|
Thanks for the thorough review, Ed. Addressed below by theme.
|
jan-elastic
left a comment
jan-elastic
left a comment
There was a problem hiding this comment.
LGTM. This seems super solid and a huge improvement over the current security model.
Left a bunch of small comments, but nothing blocking.
For the record: I didn't completely parse the syscall lists (I don't know what half of them are, nor why they're needed). Should I invest some time in that?
| return joined.empty() ? "(none)" : joined; | ||
| } | ||
|
|
||
| std::string joinStrings(const std::vector<std::string>& values) { |
There was a problem hiding this comment.
let's do
template <typename Container>
std::string joinStrings(const Container& values)
instead of the duplication
| std::vector<std::string> m_PipeDirAliasMappings; | ||
| }; | ||
|
|
||
| SArgDirExtraction extractArgDirs(const std::vector<std::string>& args) { |
There was a problem hiding this comment.
please document what this method does
|
|
||
| SArgDirExtraction extractArgDirs(const std::vector<std::string>& args) { | ||
| SArgDirExtraction extraction; | ||
| for (const auto& arg : args) { |
There was a problem hiding this comment.
why auto?
I think
for (const string& ...)
is a lot easier to read
|
|
||
| std::string path = arg.substr(eqPos + 1); | ||
| size_t lastSlash = path.rfind('/'); | ||
| if (lastSlash == std::string::npos || lastSlash == 0) { |
There was a problem hiding this comment.
npos isn't possible, because it starts with a slash
| return formatted.str(); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
why close #ifdef SANDBOX2_AVAILABLE and open a new one?
| TStrVec effectiveArgs; | ||
| effectiveArgs.reserve(args.size()); | ||
| for (const auto& arg : args) { | ||
| if (arg != "--disableSandbox") { |
There was a problem hiding this comment.
can we define this constant nowhere? it's used across multiple files
| ("cacheMemorylimitBytes", boost::program_options::value<std::size_t>(), | ||
| "Optional memory in bytes that the inference cache can use - default is 0 which disables caching") | ||
| ("validElasticLicenseKeyConfirmed", boost::program_options::value<bool>(), | ||
| ("validElasticLicenseKeyConfirmed", boost::program_options::value<bool>()->implicit_value(true), |
| boost::program_options::store(parsed, vm); | ||
| // Workaround for Sandbox2: if argv[0] is an option (Sandbox2 sets it incorrectly), | ||
| // parse it as an option using a vector of strings | ||
| if (argc > 0 && std::string(argv[0]).substr(0, 2) == "--") { |
There was a problem hiding this comment.
I think c++20 has starts_with("--")
| .options(desc) | ||
| .run(); | ||
| boost::program_options::store(parsed, vm); | ||
| // Workaround for Sandbox2: if argv[0] is an option (Sandbox2 sets it incorrectly), |
There was a problem hiding this comment.
that's strange? it that a known bug? it so, please add a reference for tracking?
|
|
||
| #ifdef SANDBOX2_AVAILABLE | ||
| //! Builds the syscall and filesystem policy for a sandboxed pytorch_inference. | ||
| //! Keep the syscall allowlist in sync with lib/seccomp/CSystemCallFilter_Linux.cc. |
There was a problem hiding this comment.
The file is called: CPytorchInferenceSyscallAllowlist.h
5 participants
PyTorch inference runs untrusted TorchScript models supplied by Elasticsearch users, so the native process must be treated as hostile. Today
pytorch_inferencerelies largely on in-process seccomp and graph validation, but that still leaves a large syscall and filesystem surface exposed inside the same address space as libtorch. This PR hardens the Linux production path by spawningpytorch_inferenceinside Google Sandbox2 from the ML controller, so isolation, syscall policy, and filesystem access are enforced before the model binary starts executing.The implementation lives primarily in
CDetachedProcessSpawner_Linux: controller-spawnedpytorch_inferenceprocesses get a Sandbox2 policy tailored to the real ES wire-up (named pipes, restore streams, libtorch threading, and long-lived daemon behaviour), while other controller children continue to use the existingposix_spawnpath. Build support vendors Abseil and the Sandboxed API on Linux, wires them into CMake, and adds the required license files. Supporting changes cover controller/pytorch command-line handling, clearer IO setup errors, seccomp filter alignment for the legacy path, and a gated--skipModelValidationbuild option that stays off in distributed builds.Validation adds Linux unit coverage for the spawner and Sandbox2 spawn path, extends the existing evil-model tests, and introduces an end-to-end attack-defense integration test run from the Docker test entrypoint. CI debugging during development surfaced several practical constraints—
clone3on older build headers, futex operations under load, FIFO visibility across mount namespaces, and Sandbox2’s default wall-time limits on a daemon process—and the policy and spawner logic were adjusted accordingly so sandboxed inference can start and stay up in real ES deployments.Review sequence recommendation
Review in this order (foundation → integration → tests → docs):
ES companion PRs: