[ML] Add Sandbox2 security integration for PyTorch inference - #2873
[ML] Add Sandbox2 security integration for PyTorch inference#2873valeriy42 wants to merge 69 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.
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
There was a problem hiding this comment.
Done — replaced the duplicated helpers with a single joinForLog template delegating to CStringUtils::join in lib/sandbox/CPytorchInferenceSandboxPolicy_Linux.cc.
| 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
There was a problem hiding this comment.
Done — extractArgDirs is documented in include/sandbox/CPytorchInferenceSandboxPolicy.h (moved with the MlSandbox extraction).
|
|
||
| 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
There was a problem hiding this comment.
Leaving as const auto& here — the container element type is already explicit in the function signature (const std::vector<std::string>& args), and auto deduces to const std::string&.
|
|
||
| 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
There was a problem hiding this comment.
Agreed — simplified to if (lastSlash == 0) in both CPytorchInferenceSandboxPolicy.cc and _Linux.cc.
| return formatted.str(); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
why close #ifdef SANDBOX2_AVAILABLE and open a new one?
There was a problem hiding this comment.
Resolved by the MlSandbox extraction — policy code now lives in a single #ifdef SANDBOX2_AVAILABLE block in CPytorchInferenceSandboxPolicy_Linux.cc.
| 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
There was a problem hiding this comment.
--disableSandbox stripping is centralized in bin/controller/CProcessSpawnerRouter.cc; core no longer parses it.
There was a problem hiding this comment.
--disableSandbox stripping is centralized in bin/controller/CProcessSpawnerRouter.cc; core no longer parses it.
| ("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), |
There was a problem hiding this comment.
Reverted to value<bool>() without implicit_value(true) to match main and the ES companion PR (--validElasticLicenseKeyConfirmed=true). The flag-only form was a sandbox-era workaround, no longer needed.
There was a problem hiding this comment.
Reverted to value<bool>() without implicit_value(true) to match main and the ES companion PR (--validElasticLicenseKeyConfirmed=true). The flag-only form was a sandbox-era workaround, no longer needed.
| 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("--")
There was a problem hiding this comment.
The argv workaround that used substr(0, 2) == "--" was removed entirely in 3dc4a56; the spawner now supplies a real program path as argv[0].
There was a problem hiding this comment.
The argv workaround that used substr(0, 2) == "--" was removed entirely in 3dc4a56; the spawner now supplies a real program path as argv[0].
| .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?
There was a problem hiding this comment.
Same — workaround deleted rather than documented. CSandboxedProcessSpawner_Linux.cc builds fullArgs[0] = processPath, so the parser no longer needs a special case.
There was a problem hiding this comment.
Same — workaround deleted rather than documented. CSandboxedProcessSpawner_Linux.cc builds fullArgs[0] = processPath, so the parser no longer needs a special case.
|
|
||
| #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
There was a problem hiding this comment.
Updated the sync comment in include/sandbox/CPytorchInferenceSandboxPolicy.h to reference include/seccomp/CPytorchInferenceSyscallAllowlist.h as the canonical list.
There was a problem hiding this comment.
Updated the sync comment in include/sandbox/CPytorchInferenceSandboxPolicy.h to reference include/seccomp/CPytorchInferenceSyscallAllowlist.h as the canonical list.
Move sandbox spawn, policy, and diagnostics out of lib/core into a new MlSandbox library, add a controller spawner router for the kill switch, and expand sandbox unit/CI coverage with enforced vs fail_closed probes.
|
@edsavage — follow-up now that the restructuring is pushed ( The Jul 28 reply described the intended changes; this commit lands them on Layering (your main concern)
Duplication
Nits from your comment
Tests / CI
Regression fix
PR description updated to match the new layout. Happy to tackle |
Remove the unsupported TMPDIR forkserver workaround, simplify spawn-context logging, restore explicit license-flag parsing, and clean up policy helper/docs references.
…stub
Restore the three pieces the split build.sh/run_tests.sh CI path never
inherited from the old combined build_and_test.sh flow:
- run_tests.sh: export ML_SANDBOX2_EXPECT=fail_closed on Linux (x86_64
in-image and inside the aarch64 test container) and re-run the sandbox
suite on the aarch64 host with ML_SANDBOX2_EXPECT=enforced, matching the
CSandboxedProcessSpawnerTest env contract.
- lib/sandbox/CMakeLists.txt: link ${ZLIB_LIBRARIES} into MlSandbox so the
controller and ml_test_sandbox resolve uncompress (libunwind_ptrace via
sandbox2) on aarch64.
- CPytorchInferenceSandboxPolicy_Windows.cc: no-op extractArgDirs stub so
Windows stops compiling the POSIX generic file (realpath/PATH_MAX).
Make SANDBOX_EXPORT empty for the static MlSandbox library so Windows controller links logSandbox2EnvironmentSelfCheck directly, propagate zlib via sandbox2::sandbox2 INTERFACE for aarch64 libunwind_ptrace, and set ML_SANDBOX2_EXPECT=enforced on Linux x86_64 test agents that have user ns.
Revert target_link_libraries on sandbox2::sandbox2 (CMake ALIAS targets reject INTERFACE edits). Propagate zlib from MlSandbox with PUBLIC --no-as-needed so aarch64 controller links uncompress after libunwind.
edsavage
left a comment
edsavage
left a comment
There was a problem hiding this comment.
LGTM
A few non-blocking follow-ups, just so they don't get lost:
CTrackerThread / setupFileActionsdedup across the macOS/Windows spawners.- Narrowing the /etc, /sys bind mounts and reconsidering
__NR_connectonce the drift guard can be updated in tandem. - The slim in-Docker policy-violation smoke, if the aarch64-outside-Docker enforcing test proves flaky on any agents.
One thing worth confirming before merge: has the aarch64 enforcing run been stable across your CI agents?
Keep -lz after sandbox2 via a non-deduplicable -Wl group so aarch64 controller links uncompress. Treat mount(proc) EPERM as unavailable in the user-ns probe and expect fail_closed on x86_64 k8s agents. Package pytorch_inference in the Linux test bundle for fail_closed spawn tests.
Ship libboost*.so* from the build image into the test bundle and point the host ml_test_sandbox LD_LIBRARY_PATH at those dirs. Dist-only paths fail because install_libs renames Boost to bare *.so while DT_NEEDED requires *.so.1.86.0.
Ship libstdc++/libgcc_s SONAMEs in the aarch64 test bundle (and widen *.so.* finds) so the host ml_test_sandbox re-run does not load the agent's older /lib64/libstdc++.so.6 (missing GLIBCXX_3.4.26+).
No Buildkite pipeline references this file; build_linux.json.py and build_macos.json.py use build.sh + run_tests.sh instead.
Replace the bare-host ml_test_sandbox re-run with a second docker run against ml-linux-aarch64-native-build:17 using SYS_ADMIN and relaxed seccomp/apparmor so unshare/mount(/proc) works without leaving the GCC13/Boost toolchain environment. Drop boost-host/gcc-host bundling from the aarch64 test bundle now that nothing executes off-image.
SYS_ADMIN plus unconfined seccomp/apparmor still cannot create user namespaces on aarch64 CI agents.
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.Architecture
MlSandbox(lib/sandbox/): ownsCSandboxedProcessSpawner,CPytorchInferenceSandboxPolicy, andCSandbox2Diagnostics.SANDBOX2_AVAILABLEis scoped to this library, notMlCore.CProcessSpawnerRouter(bin/controller/): controller-owned routing layer. Nominates./pytorch_inferenceexplicitly, strips--disableSandbox, and chooses sandboxed vs legacy spawn based on the ES kill switch (xpack.ml.trained_models.sandbox_enabled).CDetachedProcessSpawner(lib/core/): generic POSIXposix_spawnspawner again — nopytorch_inference-specific dispatch or Sandbox2 knowledge.pytorch_inference: runtime-only seccomp gate on Linux whenML_SANDBOXED=1; skips in-process BPF when running under Sandbox2.Build support vendors Abseil and the Sandboxed API on Linux, wires them into CMake, and adds the required license files.
Kill switch
When sandboxing cannot be enforced (e.g. user namespaces unavailable) or the operator disables it, the controller falls back to the legacy
posix_spawn+ in-process seccomp path via--disableSandbox.Testing
lib/sandbox/unittest: spawn, allowlist drift guard, fail-closed behaviour, and policy-violation differential tests.ML_SANDBOX2_EXPECT:enforcedon aarch64 CI (outside Docker);fail_closedin x86_64 Docker entrypoint.test/test_sandbox2_attack_defense.py: end-to-end attack-defense harness.CPytorchInferenceSyscallAllowlist.h.Review sequence (post-restructure)
lib/sandbox/CSandboxedProcessSpawner_Linux.cc+CPytorchInferenceSandboxPolicy_Linux.cc— spawn and policy.bin/controller/CProcessSpawnerRouter.cc— kill-switch routing.lib/core/CDetachedProcessSpawner.cc— generic legacy spawn baseline.lib/seccomp/CSystemCallFilter_Linux.cc+include/seccomp/CPytorchInferenceSyscallAllowlist.h— legacy vs Sandbox2 syscall coherence.bin/pytorch_inference/Main.cc— runtime seccomp gate.3rd_party/CMakeLists.txt,lib/sandbox/CMakeLists.txt,lib/core/CMakeLists.txt— build wiring.lib/sandbox/unittest/,test/test_sandbox2_attack_defense.py.docs/changelog/2873.yaml,docs/sandbox2_production_failure_modes.md.ES companion PRs