1.2: Sandboxing #7

Merged
qpismont merged 17 commits from 1.2 into main 2026-09-20 16:58:36 +02:00
7 changed files with 81 additions and 29 deletions
Showing only changes of commit 48e09aa373 - Show all commits
-11
View File
@@ -22,21 +22,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
zlib1g \ zlib1g \
&& rm -rf /var/lib/apt/lists/* && rm -rf /var/lib/apt/lists/*
# Herald drives the container daemon through its socket (DOCKER_HOST, default
# unix:///var/run/docker.sock), so neither docker nor podman is needed here: the
# compose file mounts the socket. Reaching that socket is what this user needs,
# and the socket is root-equivalent, so either run the container as root or give
# it the socket's group, e.g. `group_add: ["<gid of the host docker group>"]`.
RUN useradd --create-home --shell /usr/sbin/nologin --uid 10001 herald
WORKDIR /app WORKDIR /app
COPY --from=builder /app/target/release/herald-server ./herald-server COPY --from=builder /app/target/release/herald-server ./herald-server
# git looks for its configuration under $HOME.
ENV HOME=/home/herald
USER herald
# Exec form, so the binary is PID 1 and receives the SIGTERM it handles to shut # Exec form, so the binary is PID 1 and receives the SIGTERM it handles to shut
# down gracefully. # down gracefully.
CMD ["./herald-server"] CMD ["./herald-server"]
+12
View File
2
@@ -61,6 +61,18 @@ Herald drives the container daemon through its socket: `DOCKER_HOST` (default
Docker-compatible socket. The repository must contain a Docker-compatible socket. The repository must contain a
`.devcontainer/devcontainer.json`. `.devcontainer/devcontainer.json`.
Herald can therefore run inside a container with only that socket mounted (no
shared workspace directory is required): the clone is streamed to the daemon over
the socket, like the build context, instead of being bind-mounted from a host
path the daemon would have to see. This is the setup the `Containerfile`
produces, e.g.:
```sh
podman run --env-file=.env -p 3001:3001 \
-v /run/user/$(id -u)/podman/podman.sock:/var/run/docker.sock \
herald:latest
```
The `runArgs` of that file are read but deliberately **not** passed to the daemon: The `runArgs` of that file are read but deliberately **not** passed to the daemon:
they come from an untrusted pull request, and one of them (`--network host`) would they come from an untrusted pull request, and one of them (`--network host`) would
attach the container to another network and quietly attach the container to another network and quietly
+14 -10
View File
@@ -112,17 +112,7 @@ impl DevContainer {
), ),
working_dir: Some(workspace_folder.clone()), working_dir: Some(workspace_folder.clone()),
host_config: Some(HostConfig { host_config: Some(HostConfig {
Review

[Élevé] Le conteneur sandbox n'a aucune limite de ressources (mémoire, CPU, processus ou taille disque). Le Dockerfile, les hooks et les commandes demandées par le modèle sont contrôlés indirectement par une PR non fiable et peuvent provoquer une fork bomb, remplir le disque ou monopoliser l'hôte. Ajouter des limites explicites dans HostConfig et prévoir aussi un nettoyage fiable en cas de dépassement.

[Élevé] Le conteneur sandbox n'a aucune limite de ressources (mémoire, CPU, processus ou taille disque). Le Dockerfile, les hooks et les commandes demandées par le modèle sont contrôlés indirectement par une PR non fiable et peuvent provoquer une fork bomb, remplir le disque ou monopoliser l'hôte. Ajouter des limites explicites dans `HostConfig` et prévoir aussi un nettoyage fiable en cas de dépassement.
binds: Some(vec![format!(
"{}:{workspace_folder}",
workspace_dir.display()
)]),
network_mode: Some(network.clone()), network_mode: Some(network.clone()),
// Le clone est monté depuis un chemin de l'hôte. Sur une distribution
// à SELinux enforcing, ce chemin n'a pas le label attendu et l'accès
// est refusé (EACCES), ce qui fait échouer tous les outils de la
// sandbox. C'est le compromis inverse de l'alternative `:Z` sur le
// montage, qui re-labellise le clone et garde le confinement SELinux.
security_opt: Some(vec![String::from("label=disable")]),
..Default::default() ..Default::default()
}), }),
..Default::default() ..Default::default()
2
@@ -141,6 +131,20 @@ impl DevContainer {
return Err(err); return Err(err);
} }
// Le clone est copié dans le container par le socket, pas monté depuis un
// chemin de l'hôte : le daemon n'a pas besoin de voir le clone pour le rendre
// visible dans la sandbox, ce qui permet à Herald de tourner dans un container
// (avec le socket monté) sans partager de dossier avec l'hôte.
if let Err(err) = runtime
.upload_directory(&name, &workspace_folder, workspace_dir)
.await
{
let _ = runtime.remove_container(&name).await;
let _ = runtime.remove_network(&network).await;
let _ = runtime.remove_image(&image_tag).await;
return Err(err);
}
let container = Container::new( let container = Container::new(
runtime.clone(), runtime.clone(),
name, name,
+1 -1
View File
@@ -1,7 +1,7 @@
//! Primitives de cycle de vie de container pour un [`DevContainer`] analysé. //! Primitives de cycle de vie de container pour un [`DevContainer`] analysé.
//! //!
//! Cette crate pilote l'API du daemon de containers pour construire l'image //! Cette crate pilote l'API du daemon de containers pour construire l'image
//! devcontainer, démarrer un container avec le workspace monté, exécuter les //! devcontainer, démarrer un container et y copier le workspace, exécuter les
//! hooks `postCreateCommand` / `postStartCommand` et lancer des commandes à //! hooks `postCreateCommand` / `postStartCommand` et lancer des commandes à
//! l'intérieur du container en cours d'exécution. //! l'intérieur du container en cours d'exécution.
//! //!
+25 -1
View File
@@ -10,7 +10,7 @@ use bollard::{
models::{BuildInfo, ContainerCreateBody, NetworkCreateRequest, NetworkDisconnectRequest}, models::{BuildInfo, ContainerCreateBody, NetworkCreateRequest, NetworkDisconnectRequest},
query_parameters::{ query_parameters::{
BuildImageOptions, CreateContainerOptions, RemoveContainerOptions, RemoveImageOptions, BuildImageOptions, CreateContainerOptions, RemoveContainerOptions, RemoveImageOptions,
StartContainerOptions, StopContainerOptions, StartContainerOptions, StopContainerOptions, UploadToContainerOptions,
}, },
}; };
use futures_util::StreamExt; use futures_util::StreamExt;
3
@@ -153,6 +153,30 @@ impl ContainerRuntime {
Ok(()) Ok(())
} }
pub(crate) async fn upload_directory(
&self,
container: &str,
destination: &str,
directory: &Path,
) -> Result<(), ContainerError> {
let options = UploadToContainerOptions {
path: String::from(destination),
..Default::default()
};
self.request(
"upload workspace",
self.docker.upload_to_container(
container,
Some(options),
body_try_stream(tar_directory_stream(directory)),
),
)
.await?;
Ok(())
}
pub(crate) async fn start_container(&self, name: &str) -> Result<(), ContainerError> { pub(crate) async fn start_container(&self, name: &str) -> Result<(), ContainerError> {
self.request( self.request(
"start container", "start container",
1
+9 -6
View File
3
@@ -35,7 +35,7 @@ pub struct SandboxConfig {
pub struct Sandbox { pub struct Sandbox {
// Owns the temporary directory; dropping it cleans up the clone. // Owns the temporary directory; dropping it cleans up the clone.
_workspace: TempDir, _workspace: TempDir,
/// Path of the clone, as bind-mounted into the container. /// Path of the clone, as copied into the container.
repo_dir: PathBuf, repo_dir: PathBuf,
container: Container, container: Container,
} }
@@ -72,17 +72,20 @@ impl Sandbox {
container, container,
}; };
sandbox.check_workspace().await?; if let Err(err) = sandbox.check_workspace().await {
Review

[Élevé] Si check_workspace() échoue, le ? abandonne Sandbox après le démarrage du conteneur, mais Container n'a pas de Drop capable de supprimer les ressources asynchrones. Le conteneur, le réseau et l'image restent donc potentiellement sur le daemon à chaque échec de vérification. Nettoyer explicitement le conteneur avant de retourner l'erreur, avec conservation de l'erreur initiale même si le nettoyage échoue.

[Élevé] Si `check_workspace()` échoue, le `?` abandonne `Sandbox` après le démarrage du conteneur, mais `Container` n'a pas de `Drop` capable de supprimer les ressources asynchrones. Le conteneur, le réseau et l'image restent donc potentiellement sur le daemon à chaque échec de vérification. Nettoyer explicitement le conteneur avant de retourner l'erreur, avec conservation de l'erreur initiale même si le nettoyage échoue.
let _ = sandbox.container.remove().await;
return Err(err);
}
Ok(sandbox) Ok(sandbox)
} }
/// Vérifie que le clone est bien visible dans le container. /// Vérifie que le clone est bien visible dans le container.
/// ///
/// Sans ce contrôle, un montage vide — le daemon ne voit pas le clone, par /// Sans ce contrôle, un workspace vide — par exemple un daemon qui n'a pas pu
/// exemple quand Herald est dans un container sur une autre machine — fait /// recevoir le clone — fait échouer chaque outil ; le modèle enchaîne alors les
/// échouer chaque outil ; le modèle enchaîne alors les appels ratés jusqu'au /// appels ratés jusqu'au budget d'itérations, sans jamais pouvoir reviewer quoi
/// budget d'itérations, sans jamais pouvoir reviewer quoi que ce soit. /// que ce soit.
async fn check_workspace(&self) -> anyhow::Result<()> { async fn check_workspace(&self) -> anyhow::Result<()> {
let workspace_folder = self.workspace_folder(); let workspace_folder = self.workspace_folder();
let probe = self let probe = self
1
+20
View File
2
@@ -41,6 +41,19 @@ fn review_tools() -> Vec<Tool> {
} }
}), }),
), ),
Tool::new(
"file_size",
"Get the size of a file inside the repository in bytes.",
json!({
"type": "object",
"properties": {
"path": {
"type": "string",
"description": "File path relative to the repository root."
}
}
}),
),
Tool::new( Tool::new(
"read_file", "read_file",
"Read the content of a text file inside the repository. Every line is \ "Read the content of a text file inside the repository. Every line is \
@@ -111,12 +124,19 @@ pub async fn dispatch(sandbox: &Sandbox, name: &str, args: &Value) -> anyhow::Re
match name { match name {
"ls" => ls(sandbox, args).await, "ls" => ls(sandbox, args).await,
qpismont marked this conversation as resolved
Review

read_file exécute cat sans aucune borne : un gros fichier (ou binaire) injecte tout son contenu dans la conversation et peut saturer le contexte et faire exploser le coût. Plafonner le nombre de lignes/octets renvoyés avec une note de troncature (idem pour grep et find, potentiellement très verbeux).

`read_file` exécute `cat` sans aucune borne : un gros fichier (ou binaire) injecte tout son contenu dans la conversation et peut saturer le contexte et faire exploser le coût. Plafonner le nombre de lignes/octets renvoyés avec une note de troncature (idem pour `grep` et `find`, potentiellement très verbeux).
"read_file" => read_file(sandbox, args).await, "read_file" => read_file(sandbox, args).await,
"file_size" => file_size(sandbox, args).await,
"grep" => grep(sandbox, args).await, "grep" => grep(sandbox, args).await,
"find" => find(sandbox, args).await, "find" => find(sandbox, args).await,
other => bail!("unknown tool `{other}`"), other => bail!("unknown tool `{other}`"),
} }
} }
async fn file_size(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
let path = resolve(sandbox, required_str(args, "path")?)?;
let size = sandbox.exec(&["du", "-b", "--", &path]).await?;
into_stdout(size)
}
async fn ls(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> { async fn ls(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
let path = resolve(sandbox, optional_str(args, "path").unwrap_or("."))?; let path = resolve(sandbox, optional_str(args, "path").unwrap_or("."))?;
let output = sandbox.exec(&["ls", "-la", "--", &path]).await?; let output = sandbox.exec(&["ls", "-la", "--", &path]).await?;
4