server: add initial tool isolation support (via docker) - #26507
Conversation
github-actions
Bot
added
documentation
|
cc @ServeurpersoCom may worth doing some security audits on this /bot review |
Automated code reviewI've reviewed the diff and surrounding code. Here is my review. SummaryThis PR adds experimental Docker-based tool isolation to Blocking(point 1) Will slow the review(point 2) The (point 3) Existing-container mode runs (point 4) (point 5) (point 6) Unrelated doc changes are bundled in. The removal of the (point 7) No linked issue. This is a nontrivial feature (new runtime subsystem, new public header option, new request header, container lifecycle management). Nits(point 8) (point 9) The docker (point 10) (point 11) Minor: This review was generated automatically by pi coding agent using |
Now we're getting down to serious business! I'd really prefer Podman over Docker, but the advantage of Docker is that it might make things easier on Windows! We'll take a look at that, I've got the means to test it out, too! Off the top of my head, without having read the code, Docker requires root access, so it isn't secure by design; however, it does simplify things, and the isolation is more than sufficient for local LLMs especially since running Podman in rootless mode still requires root privileges for the initial installation anyway. |
|
yes, agree on podman, I planned to add it via a follow-up PR. it should be as simple as changing some command templates (and plus, we can also support for now, let's focus on having a solid base implementation first |
| " 'docker:<image>': spin up a new Docker container and reuse it for all invocations, clean up on server exit\n" | ||
| " 'docker-container:<id>': use an existing Docker container by ID, won't stop on server exit\n", |
There was a problem hiding this comment.
we can have podman: and ssh: in near future
There was a problem hiding this comment.
Idea :
SSH connector is exactly what would let me run this (and always have build-in tool enabled) at home and actually test it, without polluting my production box or bringing KVM into the picture.
SSH solves two things at once. On Windows it removes the whole WSL2 cross platform story that Docker drags in, since OpenSSH ships with the system and nothing else needs installing. And it makes the runtime at the far end somebody else's problem: docker, podman, VMware, KVM, a spare Raspberry Pi, whatever the user already runs.
That is really why I would not put ssh: next to docker: and podman: as a third sibling. It is not another runtime, it is a transport, and the two compose: podman on a remote host is what I actually want. Worth settling before the grammar freezes.
A small SSH only PR would have been a must, and quick to merge.
WDYT ?
| bool ok = shell_run({"sh", "-c", "mkdir -p \"$(dirname \"$1\")\"", "_", abs_path}); | ||
| if (ok) { | ||
| auto res = run_subprocess( | ||
| {"docker", "cp", tmp.string(), container_id + ":" + abs_path}, |
There was a problem hiding this comment.
for podman and other runtime, it should be as simple as swapping out this command template
|
I'm taking a little time to set up a KVM sandbox on my host server |
|
Merged master and : tools_io_docker::list_files() becomes list_entries(), with a new find_entries(), because master changed the abstract interface in the meantime. git ls-files first when only files are asked for, otherwise a find pass in the container, junk directories listed but never descended into, and truncated wired to the docker exec timeout. Three details worth mentioning in passing: the console_output_to_utf8() calls moved into run_subprocess() since you factored out a body that carried them, the two versions of get_info combined, and the junk directory list now duplicated between tools_io_basic and the docker class. |
| // the isolate is created, mounted, and torn down externally by the caller | ||
| // it must provide a POSIX environment: sh, cat, wc, mkdir, dirname, find, timeout | ||
| class tools_io_isolate : public tools_io { | ||
| public: |
There was a problem hiding this comment.
@ServeurpersoCom I abstract POSIX tool wrapper to this tools_io_isolate, so that SSH and podman support can be added in follow-up PR without duplicating too much code from docker code path
If you are interested, you can start working with tool_io_ssh / tool_io_podman in a dedicated branch, I will have a look after the current PR is merged
There was a problem hiding this comment.
also just note that for SSH, we currently only support linux destination, not sure if it worth expanding the support in the future
|
I'm doing my tinkering here; SSH is working (still need to check how robust it is against disconnections), and Podman is currently being tested. xsn/server_docker_isolate...ServeurpersoCom:llama.cpp:pascal/ssh-and-podman |
ServeurpersoCom
left a comment
ServeurpersoCom
left a comment
There was a problem hiding this comment.
Okay, I like the architecture. I got SSH working and first podman testing, proof that it's scalable.
Follow-up ggml-org#26507. The container runtime drives docker and podman through one implementation, so parametrize the availability helper, the container fixture and the attach test on the engine, and cover both engine prefixes in the container id injection test. Each engine skips on its own when it is not installed. The spawn cleanup test stays docker only: it recovers the spawned id from the container hostname, which docker sets to the short id and podman rootless does not guarantee. Podman keeps its coverage through the attach path.
Follow-up ggml-org#26507. create() writes over the handle it is given, so a respawn after the container died on its own leaked the pipes and the process handle of the previous one.
#26774) * server: add an ssh transport to the tools runtime --tools-runtime ssh:<target> runs the built-in tools on a remote host, where target is whatever ssh already resolves, a user@host or a config alias, so no credentials live in llama.cpp. Only build_argv and upload differ from the docker transport: the remote shell re-parses the command line, so the argv travels through shell_quote_join, and files go over scp with the same quoting on the remote path. Authentication is key-based and the host key must already be trusted, since the tools run without a console and any prompt would hang them. The target is validated before use. The spec can reach us from the x-tool-runtime header, and a leading dash would turn it into an ssh option, which is enough to run a command back on the host. Nothing is created and nothing is reclaimed, so an ssh spec goes straight to the tool call instead of through the container runtime. Note that this is remoting rather than isolation: the tools can do whatever the target account can do, and the isolation is whatever runs them on the far side. * server: support podman in the tools runtime docker and podman expose the same run, exec, cp and inspect verbs with the same argument order, so a single implementation drives both and the engine is carried by the spec prefix: podman:<image> and podman-container:<id> sit next to the docker forms. tools_io_docker becomes tools_io_container and the runtime spawner becomes server_tools_container_runtime, both holding the client binary chosen at parse time. A single parse_container_runtime() resolves every spec, so adding another engine is one string in the table. make_tools_io() now rejects the spawning forms. The spec also reaches it from the x-tool-runtime header, which is client controlled, and only the runtime that owns a container is allowed to create one: a tool call can attach to a running container, nothing more. * ./build/bin/llama-gen-docs * server: simplify the tools runtime and drop the file copy step A server_tools_runtime base with one virtual spec() replaces the container runtime and the bare spec string that ssh needed next to it, so server_tools is back to a single pointer and neither setup nor the handler tests which of the two is set. write_file used to spill its content into a temporary file on the host and copy it in, because run_subprocess had no way to feed a child. It now takes an optional stdin payload and creates the parent directory and the file in a single round trip through a shell in the isolate. That removes the upload virtual and both implementations: no more container cp or scp, no second binary on the host, no sftp subsystem on the target, no predictable temporary in a shared tmp, and none of the content reaching an argv the remote shell re-parses. It also fixes write_file over ssh, which never worked: scp speaks sftp and takes the remote path literally, so quoting it kept the quotes in the file name. Writing the payload before reading the output relies on the child draining stdin as it goes, which holds for cat, its only user today. * ./build/bin/llama-gen-docs * server: harden the tools runtime against argv injection and a stdin stall Validate the container id from x-tool-runtime and --tools-runtime the same way the ssh target already is, so an id shaped like an option (docker-container:--privileged) is rejected before it reaches the engine's exec command line instead of running against a hardened container. Feed the child's stdin after the watchdog is armed, so a transport that stalls mid-write is terminated at the deadline rather than blocking the request forever. Cover both guards and fix the unknown-scheme test, which used ssh: as its example and now names a real runtime. * tests: exercise the tools runtime tests on podman as well as docker Follow-up #26507. The container runtime drives docker and podman through one implementation, so parametrize the availability helper, the container fixture and the attach test on the engine, and cover both engine prefixes in the container id injection test. Each engine skips on its own when it is not installed. The spawn cleanup test stays docker only: it recovers the spawned id from the container hostname, which docker sets to the short id and podman rootless does not guarantee. Podman keeps its coverage through the attach path. * server: release the container handle before respawning Follow-up #26507. create() writes over the handle it is given, so a respawn after the container died on its own leaked the pipes and the process handle of the previous one. * server: trim the tools runtime comments * server: read tool output as raw bytes and harden the runtime on Windows The stdout pipe is read with read() instead of fgets(), so a chunk can hold any byte, including NUL, and still streams as soon as data is available. Past the size cap the pipe keeps draining so the child never blocks on a full pipe. Both pipe fds are forced to binary mode on Windows, where the CRT defaults them to text mode and translates line endings in both directions. Stdin is now always closed after the feed: the child reads a deterministic EOF, and the Windows docker and ssh clients stop outliving their command on a stdin pipe that never closes. The attach form of --tools-runtime has no lifecycle to own, so it becomes a static target validated once at startup. This removes the subprocess that ran on every tool call and serialized calls behind a mutex; a stopped container now surfaces the engine's own error at exec time. The cidfile path is passed as UTF-8, matching the encoding the subprocess layer expects for the CreateProcessW command line, so the spawn form works from a non-ASCII Windows profile. The SIGPIPE note in server.cpp now names the tools runtime children as well as the MCP ones. * clean up comments * less pollute global scope * nits * tests: name the container image after both engines --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
* server: add initial tool isolation support (via docker) * add docs * adapt get_info * py: fix type check * cont * separate tools_io_sandbox / tools_io_docker * rename sandbox --> isolate * x-tool-docker --> x-tool-runtime --------- Co-authored-by: Pascal <admin@serveurperso.com>
* server: add initial tool isolation support (via docker) * add docs * adapt get_info * py: fix type check * cont * separate tools_io_sandbox / tools_io_docker * rename sandbox --> isolate * x-tool-docker --> x-tool-runtime --------- Co-authored-by: Pascal <admin@serveurperso.com>
* server: add initial tool isolation support (via docker) * add docs * adapt get_info * py: fix type check * cont * separate tools_io_sandbox / tools_io_docker * rename sandbox --> isolate * x-tool-docker --> x-tool-runtime --------- Co-authored-by: Pascal <admin@serveurperso.com>
ggml-org#26774) * server: add an ssh transport to the tools runtime --tools-runtime ssh:<target> runs the built-in tools on a remote host, where target is whatever ssh already resolves, a user@host or a config alias, so no credentials live in llama.cpp. Only build_argv and upload differ from the docker transport: the remote shell re-parses the command line, so the argv travels through shell_quote_join, and files go over scp with the same quoting on the remote path. Authentication is key-based and the host key must already be trusted, since the tools run without a console and any prompt would hang them. The target is validated before use. The spec can reach us from the x-tool-runtime header, and a leading dash would turn it into an ssh option, which is enough to run a command back on the host. Nothing is created and nothing is reclaimed, so an ssh spec goes straight to the tool call instead of through the container runtime. Note that this is remoting rather than isolation: the tools can do whatever the target account can do, and the isolation is whatever runs them on the far side. * server: support podman in the tools runtime docker and podman expose the same run, exec, cp and inspect verbs with the same argument order, so a single implementation drives both and the engine is carried by the spec prefix: podman:<image> and podman-container:<id> sit next to the docker forms. tools_io_docker becomes tools_io_container and the runtime spawner becomes server_tools_container_runtime, both holding the client binary chosen at parse time. A single parse_container_runtime() resolves every spec, so adding another engine is one string in the table. make_tools_io() now rejects the spawning forms. The spec also reaches it from the x-tool-runtime header, which is client controlled, and only the runtime that owns a container is allowed to create one: a tool call can attach to a running container, nothing more. * ./build/bin/llama-gen-docs * server: simplify the tools runtime and drop the file copy step A server_tools_runtime base with one virtual spec() replaces the container runtime and the bare spec string that ssh needed next to it, so server_tools is back to a single pointer and neither setup nor the handler tests which of the two is set. write_file used to spill its content into a temporary file on the host and copy it in, because run_subprocess had no way to feed a child. It now takes an optional stdin payload and creates the parent directory and the file in a single round trip through a shell in the isolate. That removes the upload virtual and both implementations: no more container cp or scp, no second binary on the host, no sftp subsystem on the target, no predictable temporary in a shared tmp, and none of the content reaching an argv the remote shell re-parses. It also fixes write_file over ssh, which never worked: scp speaks sftp and takes the remote path literally, so quoting it kept the quotes in the file name. Writing the payload before reading the output relies on the child draining stdin as it goes, which holds for cat, its only user today. * ./build/bin/llama-gen-docs * server: harden the tools runtime against argv injection and a stdin stall Validate the container id from x-tool-runtime and --tools-runtime the same way the ssh target already is, so an id shaped like an option (docker-container:--privileged) is rejected before it reaches the engine's exec command line instead of running against a hardened container. Feed the child's stdin after the watchdog is armed, so a transport that stalls mid-write is terminated at the deadline rather than blocking the request forever. Cover both guards and fix the unknown-scheme test, which used ssh: as its example and now names a real runtime. * tests: exercise the tools runtime tests on podman as well as docker Follow-up ggml-org#26507. The container runtime drives docker and podman through one implementation, so parametrize the availability helper, the container fixture and the attach test on the engine, and cover both engine prefixes in the container id injection test. Each engine skips on its own when it is not installed. The spawn cleanup test stays docker only: it recovers the spawned id from the container hostname, which docker sets to the short id and podman rootless does not guarantee. Podman keeps its coverage through the attach path. * server: release the container handle before respawning Follow-up ggml-org#26507. create() writes over the handle it is given, so a respawn after the container died on its own leaked the pipes and the process handle of the previous one. * server: trim the tools runtime comments * server: read tool output as raw bytes and harden the runtime on Windows The stdout pipe is read with read() instead of fgets(), so a chunk can hold any byte, including NUL, and still streams as soon as data is available. Past the size cap the pipe keeps draining so the child never blocks on a full pipe. Both pipe fds are forced to binary mode on Windows, where the CRT defaults them to text mode and translates line endings in both directions. Stdin is now always closed after the feed: the child reads a deterministic EOF, and the Windows docker and ssh clients stop outliving their command on a stdin pipe that never closes. The attach form of --tools-runtime has no lifecycle to own, so it becomes a static target validated once at startup. This removes the subprocess that ran on every tool call and serialized calls behind a mutex; a stopped container now surfaces the engine's own error at exec time. The cidfile path is passed as UTF-8, matching the encoding the subprocess layer expects for the CreateProcessW command line, so the spawn form works from a non-ASCII Windows profile. The SIGPIPE note in server.cpp now names the tools runtime children as well as the MCP ones. * clean up comments * less pollute global scope * nits * tests: name the container image after both engines --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
* server: add initial tool isolation support (via docker) * add docs * adapt get_info * py: fix type check * cont * separate tools_io_sandbox / tools_io_docker * rename sandbox --> isolate * x-tool-docker --> x-tool-runtime --------- Co-authored-by: Pascal <admin@serveurperso.com>
ggml-org#26774) * server: add an ssh transport to the tools runtime --tools-runtime ssh:<target> runs the built-in tools on a remote host, where target is whatever ssh already resolves, a user@host or a config alias, so no credentials live in llama.cpp. Only build_argv and upload differ from the docker transport: the remote shell re-parses the command line, so the argv travels through shell_quote_join, and files go over scp with the same quoting on the remote path. Authentication is key-based and the host key must already be trusted, since the tools run without a console and any prompt would hang them. The target is validated before use. The spec can reach us from the x-tool-runtime header, and a leading dash would turn it into an ssh option, which is enough to run a command back on the host. Nothing is created and nothing is reclaimed, so an ssh spec goes straight to the tool call instead of through the container runtime. Note that this is remoting rather than isolation: the tools can do whatever the target account can do, and the isolation is whatever runs them on the far side. * server: support podman in the tools runtime docker and podman expose the same run, exec, cp and inspect verbs with the same argument order, so a single implementation drives both and the engine is carried by the spec prefix: podman:<image> and podman-container:<id> sit next to the docker forms. tools_io_docker becomes tools_io_container and the runtime spawner becomes server_tools_container_runtime, both holding the client binary chosen at parse time. A single parse_container_runtime() resolves every spec, so adding another engine is one string in the table. make_tools_io() now rejects the spawning forms. The spec also reaches it from the x-tool-runtime header, which is client controlled, and only the runtime that owns a container is allowed to create one: a tool call can attach to a running container, nothing more. * ./build/bin/llama-gen-docs * server: simplify the tools runtime and drop the file copy step A server_tools_runtime base with one virtual spec() replaces the container runtime and the bare spec string that ssh needed next to it, so server_tools is back to a single pointer and neither setup nor the handler tests which of the two is set. write_file used to spill its content into a temporary file on the host and copy it in, because run_subprocess had no way to feed a child. It now takes an optional stdin payload and creates the parent directory and the file in a single round trip through a shell in the isolate. That removes the upload virtual and both implementations: no more container cp or scp, no second binary on the host, no sftp subsystem on the target, no predictable temporary in a shared tmp, and none of the content reaching an argv the remote shell re-parses. It also fixes write_file over ssh, which never worked: scp speaks sftp and takes the remote path literally, so quoting it kept the quotes in the file name. Writing the payload before reading the output relies on the child draining stdin as it goes, which holds for cat, its only user today. * ./build/bin/llama-gen-docs * server: harden the tools runtime against argv injection and a stdin stall Validate the container id from x-tool-runtime and --tools-runtime the same way the ssh target already is, so an id shaped like an option (docker-container:--privileged) is rejected before it reaches the engine's exec command line instead of running against a hardened container. Feed the child's stdin after the watchdog is armed, so a transport that stalls mid-write is terminated at the deadline rather than blocking the request forever. Cover both guards and fix the unknown-scheme test, which used ssh: as its example and now names a real runtime. * tests: exercise the tools runtime tests on podman as well as docker Follow-up ggml-org#26507. The container runtime drives docker and podman through one implementation, so parametrize the availability helper, the container fixture and the attach test on the engine, and cover both engine prefixes in the container id injection test. Each engine skips on its own when it is not installed. The spawn cleanup test stays docker only: it recovers the spawned id from the container hostname, which docker sets to the short id and podman rootless does not guarantee. Podman keeps its coverage through the attach path. * server: release the container handle before respawning Follow-up ggml-org#26507. create() writes over the handle it is given, so a respawn after the container died on its own leaked the pipes and the process handle of the previous one. * server: trim the tools runtime comments * server: read tool output as raw bytes and harden the runtime on Windows The stdout pipe is read with read() instead of fgets(), so a chunk can hold any byte, including NUL, and still streams as soon as data is available. Past the size cap the pipe keeps draining so the child never blocks on a full pipe. Both pipe fds are forced to binary mode on Windows, where the CRT defaults them to text mode and translates line endings in both directions. Stdin is now always closed after the feed: the child reads a deterministic EOF, and the Windows docker and ssh clients stop outliving their command on a stdin pipe that never closes. The attach form of --tools-runtime has no lifecycle to own, so it becomes a static target validated once at startup. This removes the subprocess that ran on every tool call and serialized calls behind a mutex; a stopped container now surfaces the engine's own error at exec time. The cidfile path is passed as UTF-8, matching the encoding the subprocess layer expects for the CreateProcessW command line, so the spawn form works from a non-ASCII Windows profile. The SIGPIPE note in server.cpp now names the tools runtime children as well as the MCP ones. * clean up comments * less pollute global scope * nits * tests: name the container image after both engines --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
2 participants
Overview
Add initial support for tool call isolation, by proxying calls to docker container via
shcomandExample usage:
For frontend change (should be planned for a future release, cc @allozaur )
x-tool-dockerexec_shelltool on host machine to list/create/delete containers on-demandRequirements