1.2: Sandboxing #7

Merged
qpismont merged 17 commits from 1.2 into main 2026-09-20 16:58:36 +02:00
23 changed files with 271 additions and 30 deletions
Showing only changes of commit 15f619ccf7 - Show all commits
+5 -6
View File
@@ -1,4 +1,4 @@
FROM debian:trixie
FROM rust:1.97-trixie
qpismont marked this conversation as resolved
Review

Épingler l'image de base par digest (ou au moins une version majeure mineure) pour la reproductibilité, et éviter d'exécuter le conteneur de développement en root si possible. Vérifier que les outils ajoutés dans cette image ne sont pas supposés présents en production (le devcontainer ne doit pas être une dépendance implicite du runtime).

Épingler l'image de base par digest (ou au moins une version majeure mineure) pour la reproductibilité, et éviter d'exécuter le conteneur de développement en root si possible. Vérifier que les outils ajoutés dans cette image ne sont pas supposés présents en production (le devcontainer ne doit pas être une dépendance implicite du runtime).
ARG USERNAME=dev
ARG USER_UID=1000
@@ -18,11 +18,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
&& rm -rf /var/lib/apt/lists/*
RUN groupadd --gid ${USER_GID:-1000} $USERNAME \
&& useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME
&& useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME \
&& rustup component add clippy
USER $USERNAME
WORKDIR /home/$USERNAME
ENV PATH="/home/${USERNAME}/.cargo/bin:${PATH}"
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain stable
1
+1
View File
@@ -19,5 +19,6 @@
},
"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/herald,type=bind",
"workspaceFolder": "/workspaces/herald",
"runArgs": ["--userns=keep-id", "--security-opt", "label=disable"],
qpismont marked this conversation as resolved
Review

L'option --security-opt label=disable désactive la sécurité SELinux, ce qui réduit l'isolation du conteneur. Pour un environnement de développement, cela peut être acceptable, mais il faut être conscient des implications de sécurité. Envisagez d'utiliser --security-opt label=type:container_runtime_t ou d'autres options moins permissives si possible.

L'option `--security-opt label=disable` désactive la sécurité SELinux, ce qui réduit l'isolation du conteneur. Pour un environnement de développement, cela peut être acceptable, mais il faut être conscient des implications de sécurité. Envisagez d'utiliser `--security-opt label=type:container_runtime_t` ou d'autres options moins permissives si possible.
Review

Indentation par tabulation incohérente avec le reste du fichier ; --security-opt label=disable désactive l'étiquetage SELinux et --userns=keep-id est spécifique à Podman. À nettoyer/justifier, d'autant que ce devcontainer sert aussi de contexte d'exécution à la sandbox.

Indentation par tabulation incohérente avec le reste du fichier ; `--security-opt label=disable` désactive l'étiquetage SELinux et `--userns=keep-id` est spécifique à Podman. À nettoyer/justifier, d'autant que ce devcontainer sert aussi de contexte d'exécution à la sandbox.
"appPort": [3000]
}
-4
View File
@@ -1,4 +0,0 @@
target/
qpismont marked this conversation as resolved Outdated
Outdated
Review

La suppression du .dockerignore réintègre target/ et .env dans le contexte envoyé au builder (buildah bud ... .), ce qui alourdit fortement le build et expose potentiellement des secrets. Conserver un .dockerignore minimal (target, .env, .git).

La suppression du `.dockerignore` réintègre `target/` et `.env` dans le contexte envoyé au builder (`buildah bud ... .`), ce qui alourdit fortement le build et expose potentiellement des secrets. Conserver un `.dockerignore` minimal (`target`, `.env`, `.git`).
Outdated
Review

La suppression complète de .dockerignore est problématique : le contexte de build inclura désormais target/, .git/, .env et autres fichiers locaux, ce qui alourdit et surtout peut faire fuiter des secrets dans l'image via une copie du contexte. Si le fichier a été remplacé ailleurs, le référencer explicitement ; sinon le restaurer.

La suppression complète de `.dockerignore` est problématique : le contexte de build inclura désormais `target/`, `.git/`, `.env` et autres fichiers locaux, ce qui alourdit et surtout peut faire fuiter des secrets dans l'image via une copie du contexte. Si le fichier a été remplacé ailleurs, le référencer explicitement ; sinon le restaurer.
Review

La suppression de .dockerignore fait que le contexte envoyé lors de buildah bud contient désormais notamment .git, les répertoires de build et le fichier .env local. Même si le Containerfile ne le copie pas explicitement, ce contexte est transmis au moteur de build et peut contenir des secrets ou devenir inutilement volumineux. Il faut conserver un .dockerignore excluant au minimum .git, .env*, target et les fichiers locaux.

La suppression de `.dockerignore` fait que le contexte envoyé lors de `buildah bud` contient désormais notamment `.git`, les répertoires de build et le fichier `.env` local. Même si le Containerfile ne le copie pas explicitement, ce contexte est transmis au moteur de build et peut contenir des secrets ou devenir inutilement volumineux. Il faut conserver un `.dockerignore` excluant au minimum `.git`, `.env*`, `target` et les fichiers locaux.
.env
.devcontainer/
docs/
-3
View File
@@ -1,3 +0,0 @@
{
"rust-analyzer.check.command": "clippy"
}
+11
View File
@@ -0,0 +1,11 @@
{
qpismont marked this conversation as resolved
Review

Ajouter la configuration Zed tout en supprimant .vscode/settings.json crée une incohérence pour les contributeurs : il vaut mieux conserver les deux (ou documenter l'éditeur recommandé) et s'assurer qu'aucun chemin absolu local ni préférence personnelle n'est committé ici.

Ajouter la configuration Zed tout en supprimant `.vscode/settings.json` crée une incohérence pour les contributeurs : il vaut mieux conserver les deux (ou documenter l'éditeur recommandé) et s'assurer qu'aucun chemin absolu local ni préférence personnelle n'est committé ici.
"lsp": {
"rust-analyzer": {
"initialization_options": {
"check": {
"command": "clippy"
}
}
}
}
}
Generated
+15 -2
View File
1
@@ -490,6 +490,18 @@ dependencies = [
"unicode-xid",
]
[[package]]
name = "devcontainer-rs"
version = "0.1.0"
dependencies = [
"anyhow",
"serde",
"serde_json",
"tempfile",
"thiserror 2.0.18",
"tokio",
]
[[package]]
name = "dispatch2"
version = "0.3.1"
@@ -787,12 +799,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a"
[[package]]
name = "herald"
version = "1.1.0"
name = "herald-server"
version = "1.2.0"
dependencies = [
"anyhow",
"axum",
"bytes",
"devcontainer-rs",
"dotenvy",
"futures-util",
"hex",
+10 -8
View File
@@ -1,12 +1,11 @@
[package]
name = "herald"
version = "1.1.0"
edition = "2024"
[workspace]
qpismont marked this conversation as resolved
Review

Passage en workspace multi-crates : vérifier que les dépendances communes sont déclarées dans [workspace.dependencies] et réutilisées avec workspace = true dans les crates, pour éviter les divergences de versions. Ajouter aussi les métadonnées de package communes (edition, license, version) via [workspace.package].

Passage en workspace multi-crates : vérifier que les dépendances communes sont déclarées dans `[workspace.dependencies]` et réutilisées avec `workspace = true` dans les crates, pour éviter les divergences de versions. Ajouter aussi les métadonnées de package communes (edition, license, version) via `[workspace.package]`.
members = [
"crates/herald-server",
"crates/devcontainer-rs",
]
resolver = "3"
[profile.release]
debug = 1
[dependencies]
[workspace.dependencies]
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
tokio = { version = "1.53", features = ["full"] }
tokio-stream = "0.1"
@@ -30,3 +29,6 @@ hex = "0.4"
bytes = "1.1"
metrics = "0.24"
metrics-exporter-prometheus = { version = "0.18", default-features = false, features = ["http-listener"] }
[profile.release]
debug = 1
+8 -5
View File
@@ -1,12 +1,15 @@
FROM rust:1.96 as builder
FROM rust:1.97-trixie as builder
qpismont marked this conversation as resolved Outdated
Outdated
Review

Incohérence de toolchain : rust:1.97-trixie ici, alors que .devcontainer/Dockerfile et .woodpecker/tests.yml utilisent rust:1.98. Aligner les versions et écrire AS en majuscules (convention Dockerfile).

Incohérence de toolchain : `rust:1.97-trixie` ici, alors que `.devcontainer/Dockerfile` et `.woodpecker/tests.yml` utilisent `rust:1.98`. Aligner les versions et écrire `AS` en majuscules (convention Dockerfile).
Outdated
Review

Build de production : privilégier un build multi-étapes pour ne pas embarquer toolchain et sources dans l'image finale, exécuter le binaire avec un utilisateur non privilégié, et épingler les dépendances de l'étape de build. Ajouter un ENTRYPOINT/CMD explicite et un label de version.

Build de production : privilégier un build multi-étapes pour ne pas embarquer toolchain et sources dans l'image finale, exécuter le binaire avec un utilisateur non privilégié, et épingler les dépendances de l'étape de build. Ajouter un `ENTRYPOINT`/`CMD` explicite et un label de version.
Outdated
Review

Épingler l'image de base par digest plutôt que par tag flottant, et vérifier que l'image de sandbox ne contient pas de secrets ni d'outils inutiles (surface d'attaque réduite, utilisateur non-root).

Épingler l'image de base par digest plutôt que par tag flottant, et vérifier que l'image de sandbox ne contient pas de secrets ni d'outils inutiles (surface d'attaque réduite, utilisateur non-root).
WORKDIR /app
COPY . .
RUN cargo build --release
COPY Cargo.toml Cargo.lock ./
COPY crates/ crates/
RUN cargo build --release --package herald-server
FROM debian:trixie-slim
WORKDIR /app
COPY --from=builder /app/target/release/herald .
CMD [ "./herald" ]
COPY --from=builder /app/target/release/herald-server .
qpismont marked this conversation as resolved Outdated
Outdated
Review

L'étage final debian:trixie-slim ne contient que le binaire, or herald-server a besoin de git (clone de la PR) et d'un binaire de runtime conteneur (docker/podman) pour exécuter la sandbox. Avec cette image, chaque revue échouera à l'exécution. Ajouter ces paquets (plus ca-certificates) ou documenter explicitement le montage du socket/runtime requis.

L'étage final `debian:trixie-slim` ne contient que le binaire, or herald-server a besoin de `git` (clone de la PR) et d'un binaire de runtime conteneur (`docker`/`podman`) pour exécuter la sandbox. Avec cette image, chaque revue échouera à l'exécution. Ajouter ces paquets (plus `ca-certificates`) ou documenter explicitement le montage du socket/runtime requis.
CMD [ "./herald-server" ]
+14
View File
@@ -0,0 +1,14 @@
[package]
name = "devcontainer-rs"
version = "0.1.0"
edition = "2024"
[dependencies]
tokio = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
anyhow = { workspace = true }
thiserror = { workspace = true }
[dev-dependencies]
tempfile = "3"
+175
View File
@@ -0,0 +1,175 @@
use std::{
collections::HashMap,
path::{Path, PathBuf},
};
use serde::Deserialize;
#[derive(Debug, Deserialize)]
pub struct DevContainerBuildSchema {
#[serde(default)]
pub dockerfile: Option<String>,
#[serde(default)]
pub args: HashMap<String, String>,
}
#[derive(Debug, Deserialize)]
pub struct DevContainerSchema {
qpismont marked this conversation as resolved Outdated
Outdated
Review

Le champ build est obligatoire, mais le schéma standard devcontainer.json permet également d'utiliser image à la place de build. Si quelqu'un utilise image, la désérialisation échouera. Envisagez de rendre build optionnel avec #[serde(default)] et de gérer le cas où ni build ni image ne sont présents.

Le champ `build` est obligatoire, mais le schéma standard devcontainer.json permet également d'utiliser `image` à la place de `build`. Si quelqu'un utilise `image`, la désérialisation échouera. Envisagez de rendre `build` optionnel avec `#[serde(default)]` et de gérer le cas où ni `build` ni `image` ne sont présents.
#[serde(default)]
pub name: Option<String>,
pub build: DevContainerBuildSchema,
#[serde(rename = "workspaceFolder", default)]
pub workspace_folder: Option<String>,
#[serde(rename = "containerEnv", default)]
pub container_env: HashMap<String, String>,
#[serde(rename = "postCreateCommand", default)]
pub post_create_command: Option<String>,
#[serde(rename = "postStartCommand", default)]
pub post_start_command: Option<String>,
}
#[derive(Debug)]
pub struct DevContainer {
/// Absolute or relative path to the Dockerfile/Containerfile to build.
pub container_file_path: PathBuf,
pub name: Option<String>,
pub build_args: HashMap<String, String>,
pub container_env: HashMap<String, String>,
pub workspace_folder: Option<String>,
pub post_create_command: Option<String>,
pub post_start_command: Option<String>,
}
#[derive(Debug, thiserror::Error)]
pub enum ParseError {
#[error("failed to read devcontainer file `{path}`: {source}")]
Read {
path: PathBuf,
source: std::io::Error,
},
#[error("invalid devcontainer JSON in `{path}`: {source}")]
Json {
path: PathBuf,
source: serde_json::Error,
},
#[error("container file `{0}` does not exist or is not a regular file")]
ContainerFileNotFound(PathBuf),
#[error("the devcontainer file path has no parent directory: `{0}`")]
InvalidDevContainerPath(PathBuf),
}
impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer {
type Error = ParseError;
fn try_from((schema, devcontainer_path): (DevContainerSchema, PathBuf)) -> Result<Self, Self::Error> {
let base_dir = devcontainer_path
.parent()
.ok_or_else(|| ParseError::InvalidDevContainerPath(devcontainer_path.clone()))?;
let container_file_path = match schema.build.dockerfile.as_deref() {
Some(file) => base_dir.join(file),
None => first_existing_container_file(base_dir),
};
if !container_file_path.is_file() {
return Err(ParseError::ContainerFileNotFound(container_file_path));
}
Ok(Self {
container_file_path,
name: schema.name,
build_args: schema.build.args,
container_env: schema.container_env,
qpismont marked this conversation as resolved Outdated
Outdated
Review

base_dir.join(schema.build.dockerfile) : un dockerfile absolu (/etc/passwd) ou contenant .. sort du dossier du devcontainer et devient le -f de docker build (lecture de fichier arbitraire côté hôte). Valider que le chemin reste bien sous base_dir (via normalize par exemple) avant de l'accepter.

`base_dir.join(schema.build.dockerfile)` : un `dockerfile` absolu (`/etc/passwd`) ou contenant `..` sort du dossier du devcontainer et devient le `-f` de `docker build` (lecture de fichier arbitraire côté hôte). Valider que le chemin reste bien sous `base_dir` (via `normalize` par exemple) avant de l'accepter.
workspace_folder: schema.workspace_folder,
post_create_command: schema.post_create_command,
post_start_command: schema.post_start_command,
})
}
}
fn first_existing_container_file(base_dir: &Path) -> PathBuf {
qpismont marked this conversation as resolved Outdated
Outdated
Review

Si aucun fichier Dockerfile ou Containerfile n'existe, la fonction retourne base_dir.join("Dockerfile"), ce qui échouera plus tard avec ContainerFileNotFound. Cela pourrait être plus explicite en retournant une Option<PathBuf> et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.

Si aucun fichier `Dockerfile` ou `Containerfile` n'existe, la fonction retourne `base_dir.join("Dockerfile")`, ce qui échouera plus tard avec `ContainerFileNotFound`. Cela pourrait être plus explicite en retournant une `Option<PathBuf>` et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.
["Dockerfile", "Containerfile"]
.iter()
.map(|filename| base_dir.join(filename))
.find(|path| path.is_file())
.unwrap_or_else(|| base_dir.join("Dockerfile"))
}
pub async fn parse(path: impl AsRef<Path>) -> Result<DevContainer, ParseError> {
qpismont marked this conversation as resolved Outdated
Outdated
Review

La fonction parse est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent TryFrom). Il serait bon d'ajouter un test asynchrone pour parse afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).

La fonction `parse` est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent `TryFrom`). Il serait bon d'ajouter un test asynchrone pour `parse` afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).
let path = path.as_ref().to_path_buf();
let contents = tokio::fs::read_to_string(&path)
.await
.map_err(|source| ParseError::Read {
path: path.clone(),
source,
})?;
let schema = serde_json::from_str::<DevContainerSchema>(&contents).map_err(|source| {
ParseError::Json {
path: path.clone(),
source,
}
})?;
DevContainer::try_from((schema, path))
}
#[cfg(test)]
mod tests {
use super::*;
use std::fs;
#[test]
fn resolves_configured_containerfile_relative_to_devcontainer_file() {
qpismont marked this conversation as resolved Outdated
Outdated
Review

Les tests sont synchrones et utilisent std::fs plutôt que tokio::fs. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme tokio::fs), les tests pourraient ne plus être représentatifs.

Les tests sont synchrones et utilisent `std::fs` plutôt que `tokio::fs`. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme `tokio::fs`), les tests pourraient ne plus être représentatifs.
let dir = tempfile::tempdir().unwrap();
let devcontainer_path = dir.path().join("devcontainer.json");
let containerfile_path = dir.path().join("Containerfile");
fs::write(&containerfile_path, "FROM alpine\n").unwrap();
let schema = DevContainerSchema {
name: Some("test".into()),
build: DevContainerBuildSchema {
dockerfile: Some("Containerfile".into()),
args: HashMap::new(),
},
workspace_folder: None,
container_env: HashMap::new(),
post_create_command: None,
post_start_command: None,
};
let config = DevContainer::try_from((schema, devcontainer_path)).unwrap();
assert_eq!(config.container_file_path, containerfile_path);
}
#[test]
fn falls_back_to_dockerfile_before_containerfile() {
let dir = tempfile::tempdir().unwrap();
let devcontainer_path = dir.path().join("devcontainer.json");
let dockerfile_path = dir.path().join("Dockerfile");
fs::write(&dockerfile_path, "FROM alpine\n").unwrap();
fs::write(dir.path().join("Containerfile"), "FROM busybox\n").unwrap();
let schema = DevContainerSchema {
name: None,
build: DevContainerBuildSchema {
dockerfile: None,
args: HashMap::new(),
},
workspace_folder: None,
container_env: HashMap::new(),
post_create_command: None,
post_start_command: None,
};
let config = DevContainer::try_from((schema, devcontainer_path)).unwrap();
assert_eq!(config.container_file_path, dockerfile_path);
}
}
+30
View File
@@ -0,0 +1,30 @@
[package]
name = "herald-server"
version = "1.2.0"
edition = "2024"
[dependencies]
reqwest = { workspace = true }
tokio = { workspace = true }
tokio-stream = { workspace = true }
tokio-util = { workspace = true }
futures-util = { workspace = true }
serde_json = { workspace = true }
serde = { workspace = true }
sentry = { workspace = true }
sentry-anyhow = { workspace = true }
openrouter-rs = { workspace = true }
dotenvy = { workspace = true }
tower = { workspace = true }
tower-http = { workspace = true }
tracing = { workspace = true }
tracing-subscriber = { workspace = true }
axum = { workspace = true }
anyhow = { workspace = true }
thiserror = { workspace = true }
ring = { workspace = true }
hex = { workspace = true }
bytes = { workspace = true }
metrics = { workspace = true }
qpismont marked this conversation as resolved
Review

tempfile = "3" est déclaré en dur alors que toutes les autres dépendances passent par [workspace.dependencies] : le placer dans le workspace pour garder une version unique et cohérente (idem pour le dev-dependency de devcontainer-rs).

`tempfile = "3"` est déclaré en dur alors que toutes les autres dépendances passent par `[workspace.dependencies]` : le placer dans le workspace pour garder une version unique et cohérente (idem pour le dev-dependency de `devcontainer-rs`).
metrics-exporter-prometheus = { workspace = true }
devcontainer-rs = { path = "../devcontainer-rs" }