From 93dd9c9581fc19c132e200da5d96495c6631f539 Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Mon, 27 Jul 2026 21:07:47 -0700 Subject: [PATCH 1/7] fix(api): return the JSON error envelope for unmatched routes Unmatched control-plane routes fell through to the proxy fallback and returned 404 with an empty body, so JSON clients (including the E2B SDK) surfaced a bare JSONDecodeError instead of "not found". Return the spec's error envelope from the fallback so unimplemented routes self-diagnose. --- src/api/proxy.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/api/proxy.rs b/src/api/proxy.rs index e9069d44..80710444 100644 --- a/src/api/proxy.rs +++ b/src/api/proxy.rs @@ -220,7 +220,18 @@ where I: AsRef + Send + Sync, { if !has_routing_header(request.headers()) { - return StatusCode::NOT_FOUND.into_response(); + // Unmatched control-plane route: return the API error envelope so + // JSON clients surface "route not found" instead of failing to parse + // an empty 404 body. + let error = agentenv_http_server::models::Error::new( + 404, + format!( + "route not found: {} {}", + request.method(), + request.uri().path() + ), + ); + return (StatusCode::NOT_FOUND, axum::Json(error)).into_response(); } let forward_path = request.uri().path().to_owned(); with_route_source( From 92a4bb8574282903905b98b61fb03f3261d7196e Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Mon, 27 Jul 2026 21:08:00 -0700 Subject: [PATCH 2/7] feat(template): E2B build-context uploads, COPY/ADD steps, and alias rebuild Close the two gaps that kept the stock E2B SDK template builder from working end to end: Dockerfiles with COPY had no build-context upload endpoint (the SDK died on a bare 404), and rebuilding an existing alias was rejected with 400. - Implement GET /templates/{templateID}/files/{hash} (201 + {present, url}, matching the E2B spec) plus a streaming PUT endpoint the returned URL points at. Archives are stored in the snapshot repository (posix_fs and oss), so any node can issue the link, accept the upload, and read it back at build time; multi-node deployments need no routing changes. The SDK PUTs with no auth headers, so the URL carries a short-lived bearer grant persisted in the shared repository, and spent grants plus expired archives are pruned opportunistically. - Execute COPY/ADD steps by rewriting the uploaded archive host-side to final absolute guest paths per Docker COPY semantics (single file, directory contents, globs, WORKDIR-relative dest, root:root ownership, --chown/--chmod), then streaming it into the build sandbox via envd and extracting with a single tar -xpf. The rewrite is a pure function with unit tests; path escapes are rejected. - Allow alias rebuilds: the alias keeps resolving to the previous template while the new build runs and moves atomically when the build commits (E2B semantics). Failed builds leave the old binding untouched, and the previous template stays addressable by id. The alias bind is now the final fallible operation of publish in both backends. - Tolerate envd's text/plain response to the files upload endpoint; the generated client expects JSON and the upload has already completed when the decode error surfaces. --- config/default.toml | 12 + docs/src/integration/e2b.md | 42 ++ src/api/build_files.rs | 216 ++++++ src/api/generated/src/apis/templates.rs | 29 + src/api/generated/src/models.rs | 158 ++++ src/api/generated/src/server/mod.rs | 172 +++++ src/api/impls/mod.rs | 4 + src/api/impls/template.rs | 93 +++ src/api/impls/template_helpers.rs | 76 +- src/api/mod.rs | 1 + src/api/openapi.yml | 48 ++ src/api/server.rs | 5 +- src/cfg.rs | 21 + src/sandbox/backend.rs | 14 + src/sandbox/envd.rs | 35 + src/sandbox/process.rs | 12 + src/snapshot/manager.rs | 8 + .../repository/backends/oss/build_files.rs | 125 ++++ src/snapshot/repository/backends/oss/mod.rs | 1 + .../repository/backends/oss/repository.rs | 198 ++++- .../repository/backends/posixfs/backend.rs | 106 ++- .../backends/posixfs/build_files.rs | 370 ++++++++++ .../repository/backends/posixfs/catalog.rs | 124 ++-- .../repository/backends/posixfs/mod.rs | 1 + src/snapshot/repository/build_files.rs | 164 +++++ src/snapshot/repository/interfaces.rs | 10 + src/snapshot/repository/mod.rs | 2 + src/template/build_spec.rs | 88 ++- src/template/builder.rs | 56 +- src/template/copy_plan.rs | 697 ++++++++++++++++++ src/template/mod.rs | 1 + src/template/runner.rs | 6 +- src/template/step_executor.rs | 181 ++++- 33 files changed, 2945 insertions(+), 131 deletions(-) create mode 100644 src/api/build_files.rs create mode 100644 src/snapshot/repository/backends/oss/build_files.rs create mode 100644 src/snapshot/repository/backends/posixfs/build_files.rs create mode 100644 src/snapshot/repository/build_files.rs create mode 100644 src/template/copy_plan.rs diff --git a/config/default.toml b/config/default.toml index 3dab300b..e41da6b8 100644 --- a/config/default.toml +++ b/config/default.toml @@ -79,6 +79,18 @@ peer_discovery_refresh_interval_secs = 5 # Timeout for custom extension HTTP calls, in milliseconds. # timeout_ms = 5000 +[template_build] +# Build-context upload settings backing the E2B SDK's COPY support +# (GET /templates/{templateID}/files/{hash} plus the returned upload URL). +# Maximum accepted size for one uploaded build-context archive, in MiB. +# files_max_upload_mib = 1024 +# How long an issued upload URL stays valid, in seconds. +# files_url_ttl_secs = 3600 +# Optional external base URL used when building upload URLs. Defaults to +# "http://{Host header}" of the upload-link request, which matches +# direct-node and bundled-gateway deployments. +# public_base_url = "https://agentenv.example.com" + [cluster] # Shared gRPC endpoint for cluster-level services such as scheduler heartbeat # reporting and P2P peer discovery (e.g. "http://127.0.0.1:9090"). diff --git a/docs/src/integration/e2b.md b/docs/src/integration/e2b.md index 6a6db325..99738d2c 100644 --- a/docs/src/integration/e2b.md +++ b/docs/src/integration/e2b.md @@ -97,6 +97,48 @@ sandbox.beta_pause() sandbox.kill() ``` +### Template builds + +The SDK's template builder works against AgentENV, including Dockerfiles with +`COPY`: + +```python +import asyncio +from e2b import AsyncTemplate, Template + +template = Template(file_context_path=".").from_dockerfile( + """ + FROM ubuntu:24.04 + COPY requirements.txt /opt/app/requirements.txt + RUN apt-get update && apt-get install -y python3 + """ +) +asyncio.run(AsyncTemplate.build(template=template, alias="my-template")) +``` + +How `COPY` works: for each `COPY` instruction the SDK requests an upload link +(`GET /templates/{templateID}/files/{hash}`), `PUT`s a tar archive of the +matching context files to the returned bearer upload URL, and references the +archive by `filesHash` when it starts the build. AgentENV stores the archives +in the snapshot repository (shared across nodes) and extracts them inside the +build sandbox. + +Requirements and behavior notes: + +- The base image must provide `/bin/bash` (already required for `RUN` steps) + and `tar` for `COPY` steps. +- Copied files are owned by `root:root` like Docker's `COPY` default; + `COPY --chown=user:group` is applied with `chown -R` after extraction, so + the user must exist in the image. +- Write directory destinations with a trailing slash (`COPY app.py /opt/`). + Docker's special case of copying a single file onto an existing directory + named without a trailing slash (`COPY app.py /opt`) is not supported and + fails the build with a clear error. +- Rebuilding an existing alias is allowed: the alias keeps pointing at the + previous template while the new build runs and moves to the new template + when the build commits (E2B semantics). The previous template stays + addressable by ID. A failed rebuild leaves the alias untouched. + ## E2B CLI AgentENV is compatible with the E2B CLI, but we recommend using the diff --git a/src/api/build_files.rs b/src/api/build_files.rs new file mode 100644 index 00000000..cacd7ca7 --- /dev/null +++ b/src/api/build_files.rs @@ -0,0 +1,216 @@ +//! Hand-written upload endpoint for template build-context archives. +//! +//! `GET /templates/{templateID}/files/{hash}` (generated API) hands the E2B +//! SDK a bearer URL pointing here; the SDK then `PUT`s a tar archive with no +//! authentication headers. The durable random token embedded in the URL is +//! therefore the credential, and this route stays outside the generated +//! router so the archive can stream to disk instead of buffering in memory. + +use axum::extract::{Path, Request, State}; +use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; +use axum::routing::put; +use axum::{Json, Router}; +use futures::StreamExt; +use tokio::io::AsyncWriteExt; +use tracing::{debug, warn}; + +use agentenv_http_server::models; + +use super::ApiImpl; +use crate::cfg::ConfigManager; +use crate::snapshot::repository::build_files::is_valid_build_files_hash; + +pub(crate) fn router(api_impl: I) -> Router +where + I: AsRef + Clone + Send + Sync + 'static, +{ + Router::new() + .route( + "/templates/{template_id}/files/{hash}/content", + put(upload_build_archive::), + ) + .with_state(api_impl) +} + +struct UploadQuery { + expires: i64, + token: String, +} + +fn parse_upload_query(query: Option<&str>) -> Option { + let query = query?; + let mut expires: Option = None; + let mut token: Option = None; + for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { + match key.as_ref() { + "expires" => expires = value.parse().ok(), + "token" => token = Some(value.into_owned()), + _ => {} + } + } + Some(UploadQuery { + expires: expires?, + token: token?, + }) +} + +fn error_response(code: StatusCode, message: impl Into) -> Response { + ( + code, + Json(models::Error::new(code.as_u16() as i32, message.into())), + ) + .into_response() +} + +async fn upload_build_archive( + State(api_impl): State, + Path((template_id, hash)): Path<(String, String)>, + request: Request, +) -> Response +where + I: AsRef + Clone + Send + Sync + 'static, +{ + let api: &ApiImpl = api_impl.as_ref(); + + if !is_valid_build_files_hash(&hash) { + return error_response( + StatusCode::BAD_REQUEST, + format!("invalid build files hash '{hash}'"), + ); + } + let Some(store) = api.snapshot_manager().template_build_files() else { + return error_response( + StatusCode::BAD_REQUEST, + "the configured snapshot backend does not support build-context uploads", + ); + }; + let Some(query) = parse_upload_query(request.uri().query()) else { + return error_response( + StatusCode::UNAUTHORIZED, + "upload URL is missing the expires/token query parameters", + ); + }; + + let now_unix = chrono::Utc::now().timestamp(); + let authorized = match store + .validate_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) + .await + { + Ok(authorized) => authorized, + Err(error) => { + warn!(error = %error, "failed to validate build-file upload grant"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to validate upload grant", + ); + } + }; + if !authorized { + return error_response( + StatusCode::UNAUTHORIZED, + "upload grant is invalid or expired; request a fresh upload link", + ); + } + + let max_bytes = ConfigManager::global_config() + .template_build + .files_max_upload_mib + .saturating_mul(1024 * 1024); + + let staged = match tempfile::NamedTempFile::new() { + Ok(staged) => staged, + Err(error) => { + warn!(error = %error, "failed to create staging file for build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + ); + } + }; + let staged_path = staged.path().to_path_buf(); + + let mut file = match tokio::fs::File::create(&staged_path).await { + Ok(file) => file, + Err(error) => { + warn!(error = %error, "failed to open staging file for build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + ); + } + }; + + let mut total: u64 = 0; + let mut stream = request.into_body().into_data_stream(); + while let Some(chunk) = stream.next().await { + let chunk = match chunk { + Ok(chunk) => chunk, + Err(error) => { + debug!(error = %error, "build archive upload stream aborted"); + return error_response( + StatusCode::BAD_REQUEST, + "failed to read the uploaded archive body", + ); + } + }; + total += chunk.len() as u64; + if total > max_bytes { + return error_response( + StatusCode::PAYLOAD_TOO_LARGE, + format!("build archive exceeds the configured limit of {max_bytes} bytes"), + ); + } + if let Err(error) = file.write_all(&chunk).await { + warn!(error = %error, "failed to write staged build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + ); + } + } + if let Err(error) = file.flush().await { + warn!(error = %error, "failed to flush staged build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + ); + } + drop(file); + + if let Err(error) = store.import(&hash, &staged_path).await { + warn!(error = %error, hash, "failed to import build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to store build archive", + ); + } + + debug!( + template_id, + hash, + bytes = total, + "stored build-context archive" + ); + StatusCode::OK.into_response() +} + +#[cfg(test)] +mod tests { + use super::parse_upload_query; + + #[test] + fn upload_query_parses_bearer_token_and_expiry() { + let query = parse_upload_query(Some("expires=1234&token=upload-token")) + .expect("query should parse"); + assert_eq!(query.expires, 1234); + assert_eq!(query.token, "upload-token"); + } + + #[test] + fn upload_query_requires_both_fields() { + assert!(parse_upload_query(Some("expires=1234")).is_none()); + assert!(parse_upload_query(Some("token=upload-token")).is_none()); + assert!(parse_upload_query(None).is_none()); + } +} diff --git a/src/api/generated/src/apis/templates.rs b/src/api/generated/src/apis/templates.rs index a25ac144..fc727885 100644 --- a/src/api/generated/src/apis/templates.rs +++ b/src/api/generated/src/apis/templates.rs @@ -64,6 +64,22 @@ pub enum TemplatesTemplateIdDeleteResponse { Status500_ServerError(models::Error), } +#[derive(Debug, PartialEq, Serialize, Deserialize)] +#[must_use] +#[allow(clippy::large_enum_variant)] +pub enum TemplatesTemplateIdFilesHashGetResponse { + /// Successfully returned the upload link + Status201_SuccessfullyReturnedTheUploadLink(models::TemplateBuildFileUpload), + /// Bad request + Status400_BadRequest(models::Error), + /// Authentication error + Status401_AuthenticationError(models::Error), + /// Not found + Status404_NotFound(models::Error), + /// Server error + Status500_ServerError(models::Error), +} + #[derive(Debug, PartialEq, Serialize, Deserialize)] #[must_use] #[allow(clippy::large_enum_variant)] @@ -187,6 +203,19 @@ pub trait Templates: path_params: &models::TemplatesTemplateIdDeletePathParams, ) -> Result; + /// Template build file upload link. + /// + /// TemplatesTemplateIdFilesHashGet - GET /templates/{templateID}/files/{hash} + async fn templates_template_id_files_hash_get( + &self, + + method: &Method, + host: &Host, + cookies: &CookieJar, + claims: &Self::Claims, + path_params: &models::TemplatesTemplateIdFilesHashGetPathParams, + ) -> Result; + /// List template builds. /// /// TemplatesTemplateIdGet - GET /templates/{templateID} diff --git a/src/api/generated/src/models.rs b/src/api/generated/src/models.rs index 8d871403..134bd3e5 100644 --- a/src/api/generated/src/models.rs +++ b/src/api/generated/src/models.rs @@ -249,6 +249,13 @@ pub struct TemplatesTemplateIdDeletePathParams { pub template_id: String, } +#[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize, validator::Validate)] +#[cfg_attr(feature = "conversion", derive(frunk::LabelledGeneric))] +pub struct TemplatesTemplateIdFilesHashGetPathParams { + pub template_id: String, + pub hash: String, +} + #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize, validator::Validate)] #[cfg_attr(feature = "conversion", derive(frunk::LabelledGeneric))] pub struct TemplatesTemplateIdGetPathParams { @@ -7649,6 +7656,157 @@ impl std::convert::TryFrom for header::IntoHeaderValue, +} + +impl TemplateBuildFileUpload { + #[allow(clippy::new_without_default, clippy::too_many_arguments)] + pub fn new(present: bool) -> TemplateBuildFileUpload { + TemplateBuildFileUpload { present, url: None } + } +} + +/// Converts the TemplateBuildFileUpload value to the Query Parameters representation (style=form, explode=false) +/// specified in https://swagger.io/docs/specification/serialization/ +/// Should be implemented in a serde serializer +impl std::fmt::Display for TemplateBuildFileUpload { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let params: Vec> = vec![ + Some("present".to_string()), + Some(self.present.to_string()), + self.url + .as_ref() + .map(|url| ["url".to_string(), url.to_string()].join(",")), + ]; + + write!( + f, + "{}", + params.into_iter().flatten().collect::>().join(",") + ) + } +} + +/// Converts Query Parameters representation (style=form, explode=false) to a TemplateBuildFileUpload value +/// as specified in https://swagger.io/docs/specification/serialization/ +/// Should be implemented in a serde deserializer +impl std::str::FromStr for TemplateBuildFileUpload { + type Err = String; + + fn from_str(s: &str) -> std::result::Result { + /// An intermediate representation of the struct to use for parsing. + #[derive(Default)] + #[allow(dead_code)] + struct IntermediateRep { + pub present: Vec, + pub url: Vec, + } + + let mut intermediate_rep = IntermediateRep::default(); + + // Parse into intermediate representation + let mut string_iter = s.split(','); + let mut key_result = string_iter.next(); + + while key_result.is_some() { + let val = match string_iter.next() { + Some(x) => x, + None => { + return std::result::Result::Err( + "Missing value while parsing TemplateBuildFileUpload".to_string(), + ); + } + }; + + if let Some(key) = key_result { + #[allow(clippy::match_single_binding)] + match key { + #[allow(clippy::redundant_clone)] + "present" => intermediate_rep.present.push( + ::from_str(val).map_err(|x| x.to_string())?, + ), + #[allow(clippy::redundant_clone)] + "url" => intermediate_rep.url.push( + ::from_str(val).map_err(|x| x.to_string())?, + ), + _ => { + return std::result::Result::Err( + "Unexpected key while parsing TemplateBuildFileUpload".to_string(), + ); + } + } + } + + // Get the next key + key_result = string_iter.next(); + } + + // Use the intermediate representation to return the struct + std::result::Result::Ok(TemplateBuildFileUpload { + present: intermediate_rep + .present + .into_iter() + .next() + .ok_or_else(|| "present missing in TemplateBuildFileUpload".to_string())?, + url: intermediate_rep.url.into_iter().next(), + }) + } +} + +// Methods for converting between header::IntoHeaderValue and HeaderValue + +#[cfg(feature = "server")] +impl std::convert::TryFrom> for HeaderValue { + type Error = String; + + fn try_from( + hdr_value: header::IntoHeaderValue, + ) -> std::result::Result { + let hdr_value = hdr_value.to_string(); + match HeaderValue::from_str(&hdr_value) { + std::result::Result::Ok(value) => std::result::Result::Ok(value), + std::result::Result::Err(e) => std::result::Result::Err(format!( + r#"Invalid header value for TemplateBuildFileUpload - value: {hdr_value} is invalid {e}"# + )), + } + } +} + +#[cfg(feature = "server")] +impl std::convert::TryFrom for header::IntoHeaderValue { + type Error = String; + + fn try_from(hdr_value: HeaderValue) -> std::result::Result { + match hdr_value.to_str() { + std::result::Result::Ok(value) => { + match ::from_str(value) { + std::result::Result::Ok(value) => { + std::result::Result::Ok(header::IntoHeaderValue(value)) + } + std::result::Result::Err(err) => std::result::Result::Err(format!( + r#"Unable to convert header value '{value}' into TemplateBuildFileUpload - {err}"# + )), + } + } + std::result::Result::Err(e) => std::result::Result::Err(format!( + r#"Unable to convert header: {hdr_value:?} to string: {e}"# + )), + } + } +} + #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize, validator::Validate)] #[cfg_attr(feature = "conversion", derive(frunk::LabelledGeneric))] pub struct TemplateBuildInfo { diff --git a/src/api/generated/src/server/mod.rs b/src/api/generated/src/server/mod.rs index 766300a6..e5439fbf 100644 --- a/src/api/generated/src/server/mod.rs +++ b/src/api/generated/src/server/mod.rs @@ -111,6 +111,10 @@ where "/templates/{template_id}/builds/{build_id}/status", get(templates_template_id_builds_build_id_status_get::), ) + .route( + "/templates/{template_id}/files/{hash}", + get(templates_template_id_files_hash_get::), + ) .route("/v2/sandboxes", get(v2_sandboxes_get::)) .route("/v2/templates", get(v2_templates_get::)) .route( @@ -4160,6 +4164,174 @@ where }) } +#[tracing::instrument(skip_all)] +fn templates_template_id_files_hash_get_validation( + path_params: models::TemplatesTemplateIdFilesHashGetPathParams, +) -> std::result::Result<(models::TemplatesTemplateIdFilesHashGetPathParams,), ValidationErrors> { + path_params.validate()?; + + Ok((path_params,)) +} +/// TemplatesTemplateIdFilesHashGet - GET /templates/{templateID}/files/{hash} +#[tracing::instrument(skip_all)] +async fn templates_template_id_files_hash_get( + method: Method, + TypedHeader(host): TypedHeader, + cookies: CookieJar, + headers: HeaderMap, + Path(path_params): Path, + State(api_impl): State, +) -> Result +where + I: AsRef + Send + Sync, + A: apis::templates::Templates + + apis::ApiKeyAuthHeader + + apis::ApiAuthBasic + + Send + + Sync, + E: std::fmt::Debug + Send + Sync + 'static, +{ + // Authentication + let claims_in_header = api_impl + .as_ref() + .extract_claims_from_header(&headers, "X-Team-ID") + .await; + let claims_in_auth_header = api_impl + .as_ref() + .extract_claims_from_auth_header(apis::BasicAuthKind::Bearer, &headers, "authorization") + .await; + let claims = None.or(claims_in_header).or(claims_in_auth_header); + let Some(claims) = claims else { + return response_with_status_code_only(StatusCode::UNAUTHORIZED); + }; + + #[allow(clippy::redundant_closure)] + let validation = tokio::task::spawn_blocking(move || { + templates_template_id_files_hash_get_validation(path_params) + }) + .await + .unwrap(); + + let Ok((path_params,)) = validation else { + return Response::builder() + .status(StatusCode::BAD_REQUEST) + .body(Body::from(validation.unwrap_err().to_string())) + .map_err(|_| StatusCode::BAD_REQUEST); + }; + + let result = api_impl + .as_ref() + .templates_template_id_files_hash_get(&method, &host, &cookies, &claims, &path_params) + .await; + + let mut response = Response::builder(); + + let resp = match result { + Ok(rsp) => match rsp { + apis::templates::TemplatesTemplateIdFilesHashGetResponse::Status201_SuccessfullyReturnedTheUploadLink + (body) + => { + let mut response = response.status(201); + { + let mut response_headers = response.headers_mut().unwrap(); + response_headers.insert( + CONTENT_TYPE, + HeaderValue::from_static("application/json")); + } + + let body_content = tokio::task::spawn_blocking(move || + serde_json::to_vec(&body).map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + })).await.unwrap()?; + response.body(Body::from(body_content)) + }, + apis::templates::TemplatesTemplateIdFilesHashGetResponse::Status400_BadRequest + (body) + => { + let mut response = response.status(400); + { + let mut response_headers = response.headers_mut().unwrap(); + response_headers.insert( + CONTENT_TYPE, + HeaderValue::from_static("application/json")); + } + + let body_content = tokio::task::spawn_blocking(move || + serde_json::to_vec(&body).map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + })).await.unwrap()?; + response.body(Body::from(body_content)) + }, + apis::templates::TemplatesTemplateIdFilesHashGetResponse::Status401_AuthenticationError + (body) + => { + let mut response = response.status(401); + { + let mut response_headers = response.headers_mut().unwrap(); + response_headers.insert( + CONTENT_TYPE, + HeaderValue::from_static("application/json")); + } + + let body_content = tokio::task::spawn_blocking(move || + serde_json::to_vec(&body).map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + })).await.unwrap()?; + response.body(Body::from(body_content)) + }, + apis::templates::TemplatesTemplateIdFilesHashGetResponse::Status404_NotFound + (body) + => { + let mut response = response.status(404); + { + let mut response_headers = response.headers_mut().unwrap(); + response_headers.insert( + CONTENT_TYPE, + HeaderValue::from_static("application/json")); + } + + let body_content = tokio::task::spawn_blocking(move || + serde_json::to_vec(&body).map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + })).await.unwrap()?; + response.body(Body::from(body_content)) + }, + apis::templates::TemplatesTemplateIdFilesHashGetResponse::Status500_ServerError + (body) + => { + let mut response = response.status(500); + { + let mut response_headers = response.headers_mut().unwrap(); + response_headers.insert( + CONTENT_TYPE, + HeaderValue::from_static("application/json")); + } + + let body_content = tokio::task::spawn_blocking(move || + serde_json::to_vec(&body).map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + })).await.unwrap()?; + response.body(Body::from(body_content)) + }, + }, + Err(why) => { + // Application code returned an error. This should not happen, as the implementation should + // return a valid response. + return api_impl.as_ref().handle_error(&method, &host, &cookies, why).await; + }, + }; + + resp.map_err(|e| { + error!(error = ?e); + StatusCode::INTERNAL_SERVER_ERROR + }) +} + #[tracing::instrument(skip_all)] fn templates_template_id_get_validation( path_params: models::TemplatesTemplateIdGetPathParams, diff --git a/src/api/impls/mod.rs b/src/api/impls/mod.rs index 1e1d50bc..454a9d29 100644 --- a/src/api/impls/mod.rs +++ b/src/api/impls/mod.rs @@ -59,6 +59,10 @@ impl ApiImpl { Arc::clone(&self.orchestrator) } + pub(crate) fn snapshot_manager(&self) -> &SnapshotManager { + &self.snapshot_manager + } + pub(crate) fn proxy_client(&self) -> &ProxyClient { &self.proxy_client } diff --git a/src/api/impls/template.rs b/src/api/impls/template.rs index bf88f6a1..3537fecb 100644 --- a/src/api/impls/template.rs +++ b/src/api/impls/template.rs @@ -17,6 +17,7 @@ use super::template_helpers::{ }; use super::ApiImpl; use crate::image::ResolvedBlockImage; +use crate::snapshot::repository::build_files::is_valid_build_files_hash; use crate::snapshot::{ CommandContext, SnapshotAlias, SnapshotId, SnapshotListFilter, SnapshotRecord, SnapshotSource, TemplateBuildErrorReason, TemplateBuildStatus, @@ -319,6 +320,98 @@ impl Templates<()> for ApiImpl { } } + async fn templates_template_id_files_hash_get( + &self, + _method: &Method, + host: &Host, + _cookies: &CookieJar, + _claims: &Self::Claims, + path_params: &models::TemplatesTemplateIdFilesHashGetPathParams, + ) -> Result { + let template_id = &path_params.template_id; + let hash = &path_params.hash; + + if !is_valid_build_files_hash(hash) { + return Ok( + TemplatesTemplateIdFilesHashGetResponse::Status400_BadRequest(Self::error( + 400, + format!("invalid build files hash '{hash}'"), + )), + ); + } + let Some(store) = self.snapshot_manager.template_build_files() else { + return Ok( + TemplatesTemplateIdFilesHashGetResponse::Status400_BadRequest(Self::error( + 400, + "the configured snapshot backend does not support build-context uploads", + )), + ); + }; + + match self.snapshot_manager.get(template_id).await { + Ok(Some(_)) => {} + Ok(None) => { + return Ok(TemplatesTemplateIdFilesHashGetResponse::Status404_NotFound( + Self::error(404, format!("template {template_id} not found")), + )); + } + Err(err) => { + return Ok( + TemplatesTemplateIdFilesHashGetResponse::Status500_ServerError( + Self::snapshot_manager_error(&err), + ), + ); + } + } + + let present = match store.exists(hash).await { + Ok(present) => present, + Err(err) => { + return Ok( + TemplatesTemplateIdFilesHashGetResponse::Status500_ServerError(Self::error( + 500, + format!("failed to check build archive: {err}"), + )), + ); + } + }; + let config = &crate::cfg::ConfigManager::global_config().template_build; + let expires = chrono::Utc::now().timestamp() + config.files_url_ttl_secs as i64; + let token = match store.create_upload_grant(template_id, hash, expires).await { + Ok(token) => token, + Err(err) => { + return Ok( + TemplatesTemplateIdFilesHashGetResponse::Status500_ServerError(Self::error( + 500, + format!("failed to prepare upload link: {err}"), + )), + ); + } + }; + + // The SDK PUTs to this URL with a bare HTTP client (no auth headers), + // so the durable bearer token in the query string is the credential. + // Reusing the Host header keeps the URL valid across gateway and + // direct-node access. + let base = config + .public_base_url + .clone() + .unwrap_or_else(|| format!("http://{host}")); + let url = format!( + "{}/templates/{template_id}/files/{hash}/content?expires={expires}&token={token}", + base.trim_end_matches('/'), + ); + + Ok( + TemplatesTemplateIdFilesHashGetResponse::Status201_SuccessfullyReturnedTheUploadLink( + models::TemplateBuildFileUpload { + present, + url: Some(url), + }, + ), + ) + } + async fn templates_get( &self, _method: &Method, diff --git a/src/api/impls/template_helpers.rs b/src/api/impls/template_helpers.rs index 35bb32a7..86c9eeb7 100644 --- a/src/api/impls/template_helpers.rs +++ b/src/api/impls/template_helpers.rs @@ -128,17 +128,6 @@ fn apply_e2b_template_step( mut spec: TemplateBuildSpec, step: &models::TemplateStep, ) -> Result { - if step - .files_hash - .as_ref() - .is_some_and(|hash| !hash.trim().is_empty()) - { - return Err(models::Error::new( - 400, - "template build filesHash/COPY support is not implemented yet".to_string(), - )); - } - let args = step.args.as_deref().unwrap_or_default(); match step.r_type.to_ascii_uppercase().as_str() { "RUN" => { @@ -222,11 +211,68 @@ fn apply_e2b_template_step( spec = spec.label(key.to_string(), pair[1].clone()); } } + // The E2B SDK resolves ADD like COPY client-side (local files only) + // and sends both with a filesHash referencing the uploaded archive. "COPY" | "ADD" => { - return Err(models::Error::new( - 400, - format!("{} template steps are not supported yet", step.r_type), - )); + let Some(files_hash) = step + .files_hash + .as_deref() + .map(str::trim) + .filter(|hash| !hash.is_empty()) + else { + return Err(models::Error::new( + 400, + format!( + "{} template steps require a filesHash referencing an uploaded build context archive", + step.r_type + ), + )); + }; + let src = args + .first() + .map(|value| value.trim()) + .filter(|value| !value.is_empty()) + .ok_or_else(|| { + models::Error::new( + 400, + format!("{} template step requires a source argument", step.r_type), + ) + })?; + let dest = args + .get(1) + .map(|value| value.trim()) + .filter(|value| !value.is_empty()) + .ok_or_else(|| { + models::Error::new( + 400, + format!( + "{} template step requires a destination argument", + step.r_type + ), + ) + })?; + let user = args + .get(2) + .map(|value| value.trim()) + .filter(|value| !value.is_empty()) + .map(str::to_string); + let mode = args + .get(3) + .map(|value| value.trim()) + .filter(|value| !value.is_empty()) + .map(|value| { + u32::from_str_radix(value, 8).map_err(|_| { + models::Error::new( + 400, + format!( + "{} template step mode '{value}' is not a valid octal mode", + step.r_type + ), + ) + }) + }) + .transpose()?; + spec = spec.copy(src, dest, files_hash, user, mode); } other => { return Err(models::Error::new( diff --git a/src/api/mod.rs b/src/api/mod.rs index 6cae7863..86bb400a 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -1,3 +1,4 @@ +mod build_files; mod impls; mod proxy; pub mod server; diff --git a/src/api/openapi.yml b/src/api/openapi.yml index f752ee8e..1f305eaa 100644 --- a/src/api/openapi.yml +++ b/src/api/openapi.yml @@ -898,6 +898,18 @@ components: type: boolean description: Whether the step should be forced to run regardless of the cache + TemplateBuildFileUpload: + description: Upload link for one build context archive, addressed by its files hash + required: + - present + properties: + present: + type: boolean + description: Whether the archive for this hash is already stored + url: + type: string + description: URL the client should PUT the tar archive to + TemplateBuildRequestV3: properties: name: @@ -2141,6 +2153,42 @@ paths: "500": $ref: "#/components/responses/500" + /templates/{templateID}/files/{hash}: + get: + summary: Template build file upload link + description: Get an upload link for a tar archive containing build context files for one COPY step + tags: [templates] + security: + - AccessTokenAuth: [] + - ApiKeyAuth: [] + - AuthProviderBearerAuth: [] + AuthProviderTeamAuth: [] + - AdminApiKeyAuth: [] + AdminTeamAuth: [] + parameters: + - $ref: "#/components/parameters/templateID" + - in: path + name: hash + required: true + schema: + type: string + description: Hash of the build context files + responses: + "201": + description: Successfully returned the upload link + content: + application/json: + schema: + $ref: "#/components/schemas/TemplateBuildFileUpload" + "400": + $ref: "#/components/responses/400" + "401": + $ref: "#/components/responses/401" + "404": + $ref: "#/components/responses/404" + "500": + $ref: "#/components/responses/500" + /templates/aliases/{alias}: get: summary: Check template alias diff --git a/src/api/server.rs b/src/api/server.rs index 2a738898..a1a39016 100644 --- a/src/api/server.rs +++ b/src/api/server.rs @@ -1,6 +1,6 @@ use axum::{middleware, routing::get, Router}; -use super::{proxy, ApiImpl}; +use super::{build_files, proxy, ApiImpl}; use crate::observability::prometheus; use agentenv_http_server::apis; use agentenv_observability::metrics_handler; @@ -23,9 +23,10 @@ where { // Keep the generated control-plane API as the primary router, then merge in // the hand-written `/proxy/*` entrypoints needed for the temporary reverse - // proxy contract. + // proxy contract and the streaming build-context upload endpoint. agentenv_http_server::server::new::(api_impl.clone()) .merge(proxy::router(api_impl.clone())) + .merge(build_files::router(api_impl.clone())) .route("/metrics", get(metrics_handler)) .layer(middleware::from_fn_with_state( api_impl, diff --git a/src/cfg.rs b/src/cfg.rs index baf32196..c1c9931a 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -113,6 +113,8 @@ pub struct AppConfig { pub network: NetworkConfig, #[config(nested)] pub custom_extension: CustomExtensionConfig, + #[config(nested)] + pub template_build: TemplateBuildConfig, } #[derive(Debug, Deserialize, Clone, Config)] @@ -475,6 +477,25 @@ pub struct CustomExtensionConfig { pub timeout_ms: u64, } +/// Settings for the template build-context upload path used by the E2B SDK's +/// `COPY` support (`GET /templates/{templateID}/files/{hash}` plus the upload +/// URL it returns). +#[derive(Debug, Config, Clone)] +pub struct TemplateBuildConfig { + /// Maximum accepted size for one uploaded build-context archive, in MiB. + #[config(default = 1024u64)] + pub files_max_upload_mib: u64, + /// How long an issued upload URL stays valid, in seconds. + #[config(default = 3600u64)] + pub files_url_ttl_secs: u64, + /// Optional external base URL (e.g. "https://agentenv.example.com") used + /// when building upload URLs. When unset, upload URLs reuse the Host + /// header of the upload-link request with plain http, which matches + /// direct-node and bundled-gateway deployments. + #[config(env = "AENV_TEMPLATE_BUILD_PUBLIC_BASE_URL", parse_env = parse_trimmed_string)] + pub public_base_url: Option, +} + #[derive(Debug, Config, Clone)] pub struct P2pConfig { #[config(default = false)] diff --git a/src/sandbox/backend.rs b/src/sandbox/backend.rs index ad04b094..8b67944d 100644 --- a/src/sandbox/backend.rs +++ b/src/sandbox/backend.rs @@ -366,4 +366,18 @@ pub trait SandboxExecutor: Send { ) -> Result { self.executor()?.start_process(cmd, args, opts).await } + + /// Upload a local file into the sandbox at `guest_path` as `username`. + /// + /// Streams through envd's files API, so the sandbox must be running. + async fn upload_file( + &self, + local_path: &std::path::Path, + guest_path: &str, + username: &str, + ) -> Result<()> { + self.executor()? + .upload_file(local_path, guest_path, username) + .await + } } diff --git a/src/sandbox/envd.rs b/src/sandbox/envd.rs index 36f8e43f..074c6a0a 100644 --- a/src/sandbox/envd.rs +++ b/src/sandbox/envd.rs @@ -79,6 +79,41 @@ impl EnvdInstance { } } + /// Uploads a local file into the guest at `guest_path` via envd's files + /// API. `username` selects the guest account envd writes as; template + /// builds pass "root" because plain OCI images have no other account. + #[tracing::instrument(skip(self, local_path))] + pub(crate) async fn upload_file( + &self, + local_path: &std::path::Path, + guest_path: &str, + username: &str, + ) -> Result<()> { + use envd::http_client::apis::files_api; + + match files_api::files_post( + &self.config, + Some(guest_path), + Some(username), + None, + None, + Some(local_path.to_path_buf()), + ) + .await + { + Ok(_) => Ok(()), + // envd currently returns a successful text/plain response for + // this endpoint, while the generated client attempts to decode + // every 2xx response as JSON. The upload has completed by the + // time this response-body error is produced. + Err(envd::http_client::apis::Error::Serde(error)) => { + debug!(%error, "ignoring envd upload response-body decoding error"); + Ok(()) + } + Err(error) => Err(anyhow!("upload file to sandbox via envd: {error}")), + } + } + #[tracing::instrument(skip(self, env_vars))] pub(crate) async fn init( &self, diff --git a/src/sandbox/process.rs b/src/sandbox/process.rs index dad3a82d..972b7220 100644 --- a/src/sandbox/process.rs +++ b/src/sandbox/process.rs @@ -218,6 +218,18 @@ impl<'a> Executor<'a> { self.start_process_inner(cmd, args, opts, true).await } + /// Upload a local file into the sandbox at `guest_path` as `username`. + pub async fn upload_file( + &self, + local_path: &std::path::Path, + guest_path: &str, + username: &str, + ) -> Result<()> { + self.envd_instance + .upload_file(local_path, guest_path, username) + .await + } + /// Start a process via the envd gRPC client and return a [`ProcessHandle`]. async fn start_process_inner( &self, diff --git a/src/snapshot/manager.rs b/src/snapshot/manager.rs index 634bf25d..5167f354 100644 --- a/src/snapshot/manager.rs +++ b/src/snapshot/manager.rs @@ -84,6 +84,14 @@ impl SnapshotManager { self.repository.create(record).await } + /// Returns the shared build-context archive store, when the configured + /// repository backend provides one. + pub fn template_build_files( + &self, + ) -> Option> { + self.repository.template_build_files() + } + #[tracing::instrument(skip(self, metadata, manifest), fields(snapshot_id = %metadata.id))] pub async fn publish( &self, diff --git a/src/snapshot/repository/backends/oss/build_files.rs b/src/snapshot/repository/backends/oss/build_files.rs new file mode 100644 index 00000000..9654fe8f --- /dev/null +++ b/src/snapshot/repository/backends/oss/build_files.rs @@ -0,0 +1,125 @@ +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use async_trait::async_trait; + +use super::client::OssClient; +use crate::snapshot::repository::build_files::{ + generate_upload_token, is_valid_build_files_hash, is_valid_upload_token, + TemplateBuildFileStore, TemplateBuildUploadGrant, +}; +use crate::snapshot::repository::{RepositoryError, RepositoryResult}; + +const BUILD_FILES_PREFIX: &str = "template-build-files"; + +/// Build-context archive store backed by the OSS repository bucket. +/// +/// Layout: `template-build-files/{hash}.tar` plus durable bearer grants under +/// `template-build-files/upload-grants/`. Retention is delegated to bucket +/// lifecycle rules; archives are cache entries the SDK re-uploads when absent. +pub(crate) struct OssTemplateBuildFileStore { + client: Arc, +} + +impl OssTemplateBuildFileStore { + pub(crate) fn new(client: Arc) -> Arc { + Arc::new(Self { client }) + } + + fn archive_key(hash: &str) -> RepositoryResult { + if !is_valid_build_files_hash(hash) { + return Err(RepositoryError::InvalidRequest { + reason: format!("invalid build files hash '{hash}'"), + }); + } + Ok(format!("{BUILD_FILES_PREFIX}/{hash}.tar")) + } + + fn grant_key(token: &str) -> Option { + is_valid_upload_token(token) + .then(|| format!("{BUILD_FILES_PREFIX}/upload-grants/{token}.json")) + } +} + +#[async_trait] +impl TemplateBuildFileStore for OssTemplateBuildFileStore { + async fn exists(&self, hash: &str) -> RepositoryResult { + let key = Self::archive_key(hash)?; + self.client + .exists(&key) + .await + .map_err(|error| RepositoryError::backend("check build archive", error)) + } + + async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()> { + let key = Self::archive_key(hash)?; + self.client + .put_file(&key, staged) + .await + .map_err(|error| RepositoryError::backend("upload build archive", error)) + } + + async fn materialize( + &self, + hash: &str, + scratch_dir: &Path, + ) -> RepositoryResult> { + let key = Self::archive_key(hash)?; + if !self + .client + .exists(&key) + .await + .map_err(|error| RepositoryError::backend("check build archive", error))? + { + return Ok(None); + } + let dest = scratch_dir.join(format!("{hash}.tar")); + self.client + .get_to_file(&key, &dest) + .await + .map_err(|error| RepositoryError::backend("download build archive", error))?; + Ok(Some(dest)) + } + + async fn create_upload_grant( + &self, + template_id: &str, + hash: &str, + expires_unix: i64, + ) -> RepositoryResult { + let token = generate_upload_token(); + let key = Self::grant_key(&token).expect("generated token is valid"); + let grant = serde_json::to_vec(&TemplateBuildUploadGrant::new( + template_id, + hash, + expires_unix, + )) + .map_err(|error| RepositoryError::backend("serialize upload grant", error))?; + self.client + .put_bytes(&key, grant) + .await + .map_err(|error| RepositoryError::backend("write upload grant", error))?; + Ok(token) + } + + async fn validate_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult { + let Some(key) = Self::grant_key(token) else { + return Ok(false); + }; + let bytes = match self.client.get_bytes(&key).await { + Ok(bytes) => bytes, + Err(error) if OssClient::is_not_found_error(&error) => return Ok(false), + Err(error) => return Err(RepositoryError::backend("read upload grant", error)), + }; + let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) + .map_err(|error| RepositoryError::backend("parse upload grant", error))?; + Ok(grant.authorizes(template_id, hash, expires_unix, now_unix)) + } +} diff --git a/src/snapshot/repository/backends/oss/mod.rs b/src/snapshot/repository/backends/oss/mod.rs index c3adede3..1578424c 100644 --- a/src/snapshot/repository/backends/oss/mod.rs +++ b/src/snapshot/repository/backends/oss/mod.rs @@ -1,3 +1,4 @@ +mod build_files; mod client; mod config; mod layout; diff --git a/src/snapshot/repository/backends/oss/repository.rs b/src/snapshot/repository/backends/oss/repository.rs index 06f747ef..f8ecbf92 100644 --- a/src/snapshot/repository/backends/oss/repository.rs +++ b/src/snapshot/repository/backends/oss/repository.rs @@ -42,6 +42,7 @@ pub(crate) struct OssSnapshotRepository { client: Arc, snapshot_image_storage: SnapshotImageStoragePolicy, acr_exporter: AcrDiskImageExporter, + build_files: Arc, } const MAX_ALIAS_BIND_ATTEMPTS: usize = 5; @@ -51,10 +52,12 @@ impl OssSnapshotRepository { client: Arc, snapshot_image_storage: SnapshotImageStoragePolicy, ) -> Self { + let build_files = super::build_files::OssTemplateBuildFileStore::new(Arc::clone(&client)); Self { client, snapshot_image_storage, acr_exporter: AcrDiskImageExporter::new(), + build_files, } } @@ -148,6 +151,15 @@ fn fallback_to_object_storage_would_mix_sources( #[async_trait] impl SnapshotRepository for OssSnapshotRepository { + fn template_build_files( + &self, + ) -> Option> { + Some(Arc::clone(&self.build_files) + as Arc< + dyn crate::snapshot::repository::TemplateBuildFileStore, + >) + } + async fn create(&self, record: SnapshotRecord) -> RepositoryResult { if !matches!(record.source, SnapshotSource::Template { .. }) { return Err(RepositoryError::InvalidRequest { @@ -164,25 +176,28 @@ impl SnapshotRepository for OssSnapshotRepository { reason: format!("snapshot '{}' already exists", record.id), }); } + // When the alias already points at a live snapshot, leave the binding + // untouched so the existing template keeps resolving while the new + // build runs; a successful publish moves the alias to the new snapshot + // (E2B rebuild semantics). + let mut bind_on_create = true; if let Some(alias) = record.alias.as_ref() { if let Some(existing) = self.load_alias_target(alias.as_ref()).await? { if existing != record.id && self.snapshot_exists(&existing).await? { - return Err(RepositoryError::AliasConflict { - alias: alias.to_string(), - existing, - new_id: record.id.clone(), - }); + bind_on_create = false; } } } self.write_record(&record).await?; - if let Some(alias) = record.alias.as_ref() { - if let Err(error) = self.bind_alias(alias.as_ref(), &record.id).await { - let _ = self - .client - .delete(&OssSnapshotArtifactLayout::record_key(&record.id)) - .await; - return Err(error); + if bind_on_create { + if let Some(alias) = record.alias.as_ref() { + if let Err(error) = self.bind_alias(alias.as_ref(), &record.id, false).await { + let _ = self + .client + .delete(&OssSnapshotArtifactLayout::record_key(&record.id)) + .await; + return Err(error); + } } } Ok(record) @@ -287,26 +302,58 @@ impl SnapshotRepository for OssSnapshotRepository { disk_publications: disk_publications.clone(), }; - // 5. Bind alias (if present) with conflict detection. + // 5. Commit the record before moving the alias. This prevents an + // alias from ever resolving to a snapshot whose catalog record + // has not been published yet. + let previous_record = self.read_record(id).await?; + let previous_alias_target = if let Some(alias) = metadata.alias.as_ref() { + match self.load_alias_target(alias.as_ref()).await? { + Some(existing) if self.snapshot_exists(&existing).await? => Some(existing), + _ => None, + } + } else { + None + }; + let record = self + .write_committed_record( + metadata.id.clone(), + metadata.alias.clone(), + metadata.resources, + committed, + metadata.source.clone(), + ) + .await?; + + // 6. Move the alias only after the new record is readable. If the + // bind fails, restore the pending record and old alias. if let Some(ref alias) = metadata.alias { - if let Err(e) = self.bind_alias(alias.as_ref(), id).await { - // Best-effort rollback. Content-addressed managed layers are intentionally left - // in place; they are shared across snapshots and require separate GC. - if let Err(error) = self.client.delete_prefix(&layout.artifact_prefix()).await { - warn!(snapshot_id = %id, error = %error, "failed to roll back snapshot artifacts after alias bind failure"); + if let Err(error) = self.bind_alias(alias.as_ref(), id, true).await { + self.restore_alias_after_failed_bind( + alias.as_ref(), + id, + previous_alias_target.as_ref(), + ) + .await; + self.restore_record_after_failed_publish(id, previous_record.as_ref()) + .await; + return Err(error); + } + if let Some(previous_id) = previous_alias_target + .as_ref() + .filter(|previous_id| *previous_id != id) + { + if let Err(error) = self.clear_record_alias(previous_id).await { + warn!( + alias = %alias, + previous_snapshot_id = %previous_id, + error = %error, + "failed to clear previous snapshot alias metadata" + ); } - return Err(e); } } - self.write_committed_record( - metadata.id.clone(), - metadata.alias.clone(), - metadata.resources, - committed, - metadata.source.clone(), - ) - .await + Ok(record) } .await; @@ -584,7 +631,8 @@ impl OssSnapshotRepository { /// Instead the algorithm is: /// 1. Read the current alias target. /// 2. If it already points to `id`, return success (idempotent). - /// 3. If it points to a live snapshot, return `AliasConflict`. + /// 3. If it points to a live snapshot: with `rebind` move the alias to + /// `id` (E2B rebuild semantics), otherwise return `AliasConflict`. /// 4. If it points to a deleted snapshot, remove the stale alias. /// 5. Write our binding unconditionally. /// 6. Read back and verify we won the race. If someone else wrote a @@ -595,7 +643,7 @@ impl OssSnapshotRepository { /// interval between our write and the subsequent read. This is weaker /// than a true CAS but sufficient for the current deployment model /// where concurrent publishes for the *same alias* are rare. - async fn bind_alias(&self, alias: &str, id: &SnapshotId) -> RepositoryResult<()> { + async fn bind_alias(&self, alias: &str, id: &SnapshotId, rebind: bool) -> RepositoryResult<()> { let key = validated_alias_key(alias)?; let payload = serde_json::to_vec(id) .map_err(|e| RepositoryError::backend("serialize alias binding", e))?; @@ -608,18 +656,19 @@ impl OssSnapshotRepository { } let still_exists = self.snapshot_exists(&existing_id).await?; - if still_exists { + if still_exists && !rebind { return Err(RepositoryError::AliasConflict { alias: alias.to_string(), existing: existing_id, new_id: id.clone(), }); } - - self.client - .delete(&key) - .await - .map_err(|e| RepositoryError::backend("delete stale alias", e))?; + if !still_exists { + self.client + .delete(&key) + .await + .map_err(|e| RepositoryError::backend("delete stale alias", e))?; + } } // Step 5: write our binding (unconditional — OSS does not @@ -669,6 +718,85 @@ impl OssSnapshotRepository { }) } + async fn restore_record_after_failed_publish( + &self, + id: &SnapshotId, + previous_record: Option<&SnapshotRecord>, + ) { + let result = match previous_record { + Some(record) => self.write_record(record).await, + None => self + .client + .delete(&OssSnapshotArtifactLayout::record_key(id)) + .await + .map_err(|error| RepositoryError::backend("remove failed snapshot record", error)), + }; + if let Err(error) = result { + warn!(snapshot_id = %id, error = %error, "failed to restore snapshot record after publish failure"); + } + } + + async fn restore_alias_after_failed_bind( + &self, + alias: &str, + id: &SnapshotId, + previous_id: Option<&SnapshotId>, + ) { + let current = match self.load_alias_target(alias).await { + Ok(current) => current, + Err(error) => { + warn!(alias, snapshot_id = %id, error = %error, "failed to inspect alias during publish rollback"); + return; + } + }; + // Do not overwrite a concurrent publisher that already moved the + // alias elsewhere. + if current.as_ref() != Some(id) { + return; + } + + let key = match validated_alias_key(alias) { + Ok(key) => key, + Err(error) => { + warn!(alias, snapshot_id = %id, error = %error, "failed to validate alias during publish rollback"); + return; + } + }; + let result = + match previous_id { + Some(previous_id) => match serde_json::to_vec(previous_id) { + Ok(payload) => { + self.client.put_bytes(&key, payload).await.map_err(|error| { + RepositoryError::backend("restore alias binding", error) + }) + } + Err(error) => Err(RepositoryError::backend( + "serialize restored alias binding", + error, + )), + }, + None => self.client.delete(&key).await.map_err(|error| { + RepositoryError::backend("remove failed alias binding", error) + }), + }; + if let Err(error) = result { + warn!(alias, snapshot_id = %id, error = %error, "failed to restore alias after publish failure"); + } + } + + /// Clears the alias field on the record that previously owned a rebound + /// alias so template listings do not report the moved name twice. + async fn clear_record_alias(&self, id: &SnapshotId) -> RepositoryResult<()> { + if let Some(mut previous) = self.read_record(id).await? { + if previous.alias.is_some() { + previous.alias = None; + previous.updated_at_unix_ms = now_unix_ms(); + self.write_record(&previous).await?; + } + } + Ok(()) + } + async fn export_managed_disk_image( &self, image_config_path: &Path, diff --git a/src/snapshot/repository/backends/posixfs/backend.rs b/src/snapshot/repository/backends/posixfs/backend.rs index 4dc7f7ba..3e90fec3 100644 --- a/src/snapshot/repository/backends/posixfs/backend.rs +++ b/src/snapshot/repository/backends/posixfs/backend.rs @@ -7,11 +7,13 @@ use tokio::task; use super::super::shared_runtime_cache_root; use super::artifacts::{CollectedBuiltArtifacts, PosixFsArtifactStore}; +use super::build_files::PosixFsTemplateBuildFileStore; use super::catalog::PosixFsCatalogStore; use super::runtime::PosixFsRuntimeResolver; use crate::image::cache::{local_image_services_from_global_config, OverlaybdLayerStore}; use crate::sandbox::FirecrackerSnapshotManifest; use crate::snapshot::artifact_cache::LocalArtifactCache; +use crate::snapshot::repository::build_files::TemplateBuildFileStore; use crate::snapshot::repository::interfaces::{SnapshotRepository, SnapshotRuntimeResolver}; use crate::snapshot::repository::{RepositoryError, RepositoryResult, SnapshotListFilter}; use crate::snapshot::types::{ @@ -72,9 +74,11 @@ impl PosixFsBackend { let runtime_cache_root = runtime_cache_root.unwrap_or_else(|| cache_root.join("runtime")); let catalog_store = Arc::new(PosixFsCatalogStore::new(root.clone())); let artifact_store = Arc::new(PosixFsArtifactStore::new(root.clone())); + let build_files = PosixFsTemplateBuildFileStore::new(&root); let repository: Arc = Arc::new(PosixFsSnapshotRepository::new( catalog_store, artifact_store, + build_files, )); let runtime_resolver: Arc = Arc::new( PosixFsRuntimeResolver::new(root, runtime_cache_root, store, cache), @@ -111,16 +115,19 @@ impl PosixFsBackend { pub(crate) struct PosixFsSnapshotRepository { catalog_store: Arc, artifact_store: Arc, + build_files: Arc, } impl PosixFsSnapshotRepository { pub(crate) fn new( catalog_store: Arc, artifact_store: Arc, + build_files: Arc, ) -> Self { Self { catalog_store, artifact_store, + build_files, } } @@ -237,6 +244,10 @@ impl SnapshotRepository for PosixFsSnapshotRepository { .await } + fn template_build_files(&self) -> Option> { + Some(Arc::clone(&self.build_files) as Arc) + } + async fn publish( &self, metadata: SnapshotPublishMetadata, @@ -399,6 +410,7 @@ mod tests { PosixFsSnapshotRepository::new( Arc::new(PosixFsCatalogStore::new(root.to_path_buf())), Arc::new(PosixFsArtifactStore::new(root.to_path_buf())), + super::super::build_files::PosixFsTemplateBuildFileStore::new(root), ) } @@ -474,39 +486,105 @@ mod tests { } #[tokio::test] - async fn failed_commit_cleans_uncommitted_snapshot_directory() { + async fn publish_rebinds_existing_alias_to_new_snapshot() { let tempdir = TempDir::new().expect("tempdir should exist"); - let repository_root = tempdir.path().to_path_buf(); let repository = test_backend(tempdir.path()).repository(); let first_id = SnapshotId::generate(); let local_artifacts = seed_built_snapshot(tempdir.path()); - let first_metadata = sample_metadata(first_id.clone(), Some("conflict")); repository - .publish(first_metadata, local_artifacts) + .publish( + sample_metadata(first_id.clone(), Some("rebind")), + local_artifacts, + ) .await .expect("first publish should work"); let second_id = SnapshotId::generate(); let local_artifacts = seed_built_snapshot(tempdir.path()); - let err = repository + repository .publish( - sample_metadata(second_id.clone(), Some("conflict")), + sample_metadata(second_id.clone(), Some("rebind")), local_artifacts, ) .await - .expect_err("second publish should fail"); + .expect("second publish should rebind the alias"); - assert!(matches!(err, RepositoryError::AliasConflict { .. })); - assert!( - !repository_root - .join("snapshots") - .join(second_id.to_string()) - .exists(), - "failed publish should not leave a committed revision directory" + let resolved = repository + .resolve_alias("rebind") + .await + .expect("resolve should work") + .expect("alias should resolve"); + assert_eq!(resolved, second_id, "alias should move to the new snapshot"); + + let previous = repository + .get(first_id.to_string().as_str()) + .await + .expect("get should work") + .expect("previous snapshot should stay addressable by id"); + assert_eq!( + previous.alias, None, + "previous snapshot should lose the rebound alias" ); } + #[tokio::test] + async fn create_keeps_existing_alias_until_new_build_commits() { + let tempdir = TempDir::new().expect("tempdir should exist"); + let repository = test_backend(tempdir.path()).repository(); + + let committed_id = SnapshotId::generate(); + let local_artifacts = seed_built_snapshot(tempdir.path()); + repository + .publish( + sample_metadata(committed_id.clone(), Some("stable")), + local_artifacts, + ) + .await + .expect("publish should work"); + + let waiting = SnapshotRecord::template_waiting( + SnapshotId::generate(), + Some(SnapshotAlias::parse("stable").expect("alias should parse")), + crate::types::SandboxResources { + cpu_count: 1, + memory_mib: 256, + disk_size_mib: 0, + }, + ); + let waiting_id = waiting.id.clone(); + repository + .create(waiting) + .await + .expect("create with an existing alias should be allowed"); + + let resolved = repository + .resolve_alias("stable") + .await + .expect("resolve should work") + .expect("alias should resolve"); + assert_eq!( + resolved, committed_id, + "alias should keep pointing at the committed snapshot while the rebuild is pending" + ); + + let local_artifacts = seed_built_snapshot(tempdir.path()); + repository + .publish( + sample_metadata(waiting_id.clone(), Some("stable")), + local_artifacts, + ) + .await + .expect("publishing the rebuild should rebind the alias"); + + let resolved = repository + .resolve_alias("stable") + .await + .expect("resolve should work") + .expect("alias should resolve"); + assert_eq!(resolved, waiting_id, "alias should move after commit"); + } + #[tokio::test] async fn delete_removes_committed_snapshot_directory() { let tempdir = TempDir::new().expect("tempdir should exist"); diff --git a/src/snapshot/repository/backends/posixfs/build_files.rs b/src/snapshot/repository/backends/posixfs/build_files.rs new file mode 100644 index 00000000..ae8d4cbf --- /dev/null +++ b/src/snapshot/repository/backends/posixfs/build_files.rs @@ -0,0 +1,370 @@ +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::{Duration, SystemTime}; + +use async_trait::async_trait; +use tokio::task; +use tracing::{debug, warn}; + +use crate::snapshot::repository::build_files::{ + generate_upload_token, is_valid_build_files_hash, is_valid_upload_token, + TemplateBuildFileStore, TemplateBuildUploadGrant, +}; +use crate::snapshot::repository::{RepositoryError, RepositoryResult}; + +/// How long imported build-context archives and upload grants are retained. +/// Archives are cache entries keyed by content hash; the SDK re-uploads any +/// archive that has been pruned, so expiry only costs one extra upload. +/// Grants expire after `template_build.files_url_ttl_secs` anyway, so this +/// only bounds how long the spent grant files linger on disk. +const BUILD_FILE_RETENTION: Duration = Duration::from_secs(7 * 24 * 60 * 60); + +const GRANTS_DIR_NAME: &str = "upload-grants"; + +/// Build-context archive store rooted on the shared POSIX repository. +/// +/// Layout: `{repository_root}/template-build-files/{hash}.tar` plus durable +/// upload grants under `upload-grants/`. Both live on the shared filesystem, +/// so every node observes the same archives and verifies the same upload URLs. +pub(crate) struct PosixFsTemplateBuildFileStore { + root: PathBuf, +} + +impl PosixFsTemplateBuildFileStore { + pub(crate) fn new(repository_root: &Path) -> Arc { + Arc::new(Self { + root: repository_root.join("template-build-files"), + }) + } + + fn archive_path(&self, hash: &str) -> RepositoryResult { + if !is_valid_build_files_hash(hash) { + return Err(RepositoryError::InvalidRequest { + reason: format!("invalid build files hash '{hash}'"), + }); + } + Ok(self.root.join(format!("{hash}.tar"))) + } + + fn ensure_root(root: &Path) -> RepositoryResult<()> { + fs::create_dir_all(root).map_err(|error| { + RepositoryError::backend( + format!("create template build files dir '{}'", root.display()), + error, + ) + }) + } + + /// Removes archives whose modification time is older than the retention + /// window. Runs opportunistically on import; failures only log. + fn prune_expired(root: &Path) { + let cutoff = SystemTime::now() - BUILD_FILE_RETENTION; + Self::prune_dir_older_than(root, "tar", cutoff); + } + + /// Removes upload grants older than the retention window. Runs + /// opportunistically whenever a new grant is written, so the grants + /// directory stays bounded by upload-link traffic; failures only log. + fn prune_expired_grants(root: &Path) { + let cutoff = SystemTime::now() - BUILD_FILE_RETENTION; + Self::prune_dir_older_than(&Self::grants_dir(root), "json", cutoff); + } + + fn prune_dir_older_than(dir: &Path, extension: &str, cutoff: SystemTime) { + let Ok(entries) = fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().is_none_or(|ext| ext != extension) { + continue; + } + let expired = entry + .metadata() + .and_then(|metadata| metadata.modified()) + .map(|modified| modified < cutoff) + .unwrap_or(false); + if expired { + if let Err(error) = fs::remove_file(&path) { + warn!( + path = %path.display(), + error = %error, + "failed to prune expired template build file" + ); + } else { + debug!(path = %path.display(), "pruned expired template build file"); + } + } + } + } + + fn grants_dir(root: &Path) -> PathBuf { + root.join(GRANTS_DIR_NAME) + } + + fn grant_path(root: &Path, token: &str) -> Option { + is_valid_upload_token(token).then(|| Self::grants_dir(root).join(format!("{token}.json"))) + } + + fn write_grant( + root: &Path, + template_id: &str, + hash: &str, + expires_unix: i64, + ) -> RepositoryResult { + let grants_dir = Self::grants_dir(root); + fs::create_dir_all(&grants_dir).map_err(|error| { + RepositoryError::backend( + format!("create upload grants dir '{}'", grants_dir.display()), + error, + ) + })?; + Self::prune_expired_grants(root); + let bytes = serde_json::to_vec(&TemplateBuildUploadGrant::new( + template_id, + hash, + expires_unix, + )) + .map_err(|error| RepositoryError::backend("serialize upload grant", error))?; + + for _ in 0..3 { + let token = generate_upload_token(); + let path = Self::grant_path(root, &token).expect("generated token is valid"); + match fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&path) + { + Ok(mut file) => { + file.write_all(&bytes) + .and_then(|()| file.sync_all()) + .map_err(|error| { + let _ = fs::remove_file(&path); + RepositoryError::backend("write upload grant", error) + })?; + return Ok(token); + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(RepositoryError::backend("create upload grant", error)), + } + } + Err(RepositoryError::Backend { + message: "failed to allocate a unique upload grant token".to_string(), + source: None, + }) + } +} + +#[async_trait] +impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { + async fn exists(&self, hash: &str) -> RepositoryResult { + let path = self.archive_path(hash)?; + task::spawn_blocking(move || path.exists()) + .await + .map_err(|error| RepositoryError::backend("join build file exists task", error)) + } + + async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()> { + let final_path = self.archive_path(hash)?; + let root = self.root.clone(); + let staged = staged.to_path_buf(); + task::spawn_blocking(move || -> RepositoryResult<()> { + Self::ensure_root(&root)?; + Self::prune_expired(&root); + // Copy into the store filesystem first (the staged file usually + // lives on node-local tmp), then rename within the store directory + // so readers only ever observe complete archives. + let store_staged = root.join(format!(".import-{}.tmp", uuid::Uuid::new_v4())); + fs::copy(&staged, &store_staged).map_err(|error| { + let _ = fs::remove_file(&store_staged); + RepositoryError::backend("copy build archive into store", error) + })?; + fs::rename(&store_staged, &final_path).map_err(|error| { + let _ = fs::remove_file(&store_staged); + RepositoryError::backend("publish build archive", error) + }) + }) + .await + .map_err(|error| RepositoryError::backend("join build file import task", error))? + } + + async fn materialize( + &self, + hash: &str, + _scratch_dir: &Path, + ) -> RepositoryResult> { + let path = self.archive_path(hash)?; + task::spawn_blocking(move || path.exists().then_some(path)) + .await + .map_err(|error| RepositoryError::backend("join build file materialize task", error)) + } + + async fn create_upload_grant( + &self, + template_id: &str, + hash: &str, + expires_unix: i64, + ) -> RepositoryResult { + let root = self.root.clone(); + let template_id = template_id.to_string(); + let hash = hash.to_string(); + task::spawn_blocking(move || Self::write_grant(&root, &template_id, &hash, expires_unix)) + .await + .map_err(|error| RepositoryError::backend("join create upload grant task", error))? + } + + async fn validate_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult { + let Some(path) = Self::grant_path(&self.root, token) else { + return Ok(false); + }; + let template_id = template_id.to_string(); + let hash = hash.to_string(); + task::spawn_blocking(move || { + let bytes = match fs::read(&path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => { + return Err(RepositoryError::backend("read upload grant", error)); + } + }; + let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) + .map_err(|error| RepositoryError::backend("parse upload grant", error))?; + Ok(grant.authorizes(&template_id, &hash, expires_unix, now_unix)) + }) + .await + .map_err(|error| RepositoryError::backend("join validate upload grant task", error))? + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + const HASH: &str = "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"; + + fn staged_file(dir: &Path, contents: &[u8]) -> PathBuf { + let path = dir.join("staged.tar"); + fs::write(&path, contents).expect("write staged file"); + path + } + + #[tokio::test] + async fn import_then_exists_and_materialize() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + assert!(!store.exists(HASH).await.expect("exists should work")); + assert_eq!( + store + .materialize(HASH, tempdir.path()) + .await + .expect("materialize should work"), + None + ); + + let staged = staged_file(tempdir.path(), b"tar-bytes"); + store + .import(HASH, &staged) + .await + .expect("import should work"); + + assert!(store.exists(HASH).await.expect("exists should work")); + let materialized = store + .materialize(HASH, tempdir.path()) + .await + .expect("materialize should work") + .expect("archive should exist"); + assert_eq!( + fs::read(materialized).expect("read materialized"), + b"tar-bytes" + ); + } + + #[tokio::test] + async fn import_rejects_invalid_hash() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let staged = staged_file(tempdir.path(), b"tar-bytes"); + + let err = store + .import("../escape", &staged) + .await + .expect_err("invalid hash should fail"); + assert!(matches!(err, RepositoryError::InvalidRequest { .. })); + } + + #[tokio::test] + async fn writing_a_grant_prunes_expired_grant_files() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + let fresh_token = store + .create_upload_grant("template", HASH, i64::MAX) + .await + .expect("fresh grant should be created"); + + // Plant a grant file that predates the retention window. + let grants_dir = tempdir + .path() + .join("template-build-files") + .join("upload-grants"); + let stale_path = grants_dir.join(format!("{}.json", generate_upload_token())); + fs::write(&stale_path, b"{}").expect("write stale grant"); + let stale_mtime = SystemTime::now() - BUILD_FILE_RETENTION - Duration::from_secs(60); + let stale_file = fs::File::options() + .write(true) + .open(&stale_path) + .expect("open stale grant"); + stale_file + .set_times(fs::FileTimes::new().set_modified(stale_mtime)) + .expect("set stale mtime"); + drop(stale_file); + + store + .create_upload_grant("template", HASH, i64::MAX) + .await + .expect("new grant should be created"); + + assert!(!stale_path.exists(), "expired grant file should be pruned"); + assert!( + store + .validate_upload_grant(&fresh_token, "template", HASH, i64::MAX, 0) + .await + .expect("validation should work"), + "unexpired grants must survive pruning" + ); + } + + #[tokio::test] + async fn upload_grant_is_shared_across_instances() { + let tempdir = TempDir::new().expect("tempdir"); + let first = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let token = first + .create_upload_grant("template", HASH, 1000) + .await + .expect("grant should be created"); + let second = PosixFsTemplateBuildFileStore::new(tempdir.path()); + assert!(second + .validate_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("grant should validate")); + assert!(!second + .validate_upload_grant(&token, "other", HASH, 1000, 999) + .await + .expect("mismatched grant should be rejected")); + assert!(!second + .validate_upload_grant(&token, "template", HASH, 1000, 1001) + .await + .expect("expired grant should be rejected")); + } +} diff --git a/src/snapshot/repository/backends/posixfs/catalog.rs b/src/snapshot/repository/backends/posixfs/catalog.rs index 6c36d6bd..ae122637 100644 --- a/src/snapshot/repository/backends/posixfs/catalog.rs +++ b/src/snapshot/repository/backends/posixfs/catalog.rs @@ -6,6 +6,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use serde::de::DeserializeOwned; use serde::Serialize; +use tracing::warn; use super::layout::PosixFsSnapshotArtifactLayout; use crate::snapshot::repository::SnapshotListFilter; @@ -79,9 +80,9 @@ impl PosixFsCatalogStore { /// /// Flow: /// 1. acquire the alias lock when an alias is present - /// 2. bind the alias - /// 3. write the commit marker - /// 4. write the committed snapshot record + /// 2. write the commit marker + /// 3. write the committed snapshot record + /// 4. atomically bind the alias as the final visible operation pub(crate) fn commit_publish( &self, session: &PublishSession, @@ -90,25 +91,48 @@ impl PosixFsCatalogStore { ) -> RepositoryResult { let now = now_unix_ms(); let snapshot_id = metadata.id.clone(); + let previous_record = self.load_record_by_id_unlocked(&snapshot_id)?; let write_result = if let Some(alias) = metadata.alias.as_ref() { self.with_alias_lock(alias, |store| { let record = store.committed_record_unlocked(&metadata, committed.clone(), now)?; let alias_path = PosixFsSnapshotArtifactLayout::alias_path(&store.root, alias); - if let Some(existing) = store.load_alias_target(alias)? { - if existing != snapshot_id { - if store.load_record_by_id_unlocked(&existing)?.is_some() { - return Err(RepositoryError::AliasConflict { - alias: alias.to_string(), - existing, - new_id: snapshot_id.clone(), - }); + let existing = store.load_alias_target(alias)?; + store.write_commit_marker(&session.snapshot_id)?; + store.write_committed_record_unlocked(&record)?; + // `write_json` uses an atomic rename. Keeping this as the final + // fallible operation means a failed rebuild leaves the old + // alias binding untouched. + store.write_json(&alias_path, &snapshot_id)?; + + if let Some(existing) = existing.filter(|existing| existing != &snapshot_id) { + // The previous snapshot stays addressable by id, so running + // sandboxes and explicit id references keep working. Alias + // metadata cleanup is best effort because the binding has + // already moved successfully. + match store.load_record_by_id_unlocked(&existing) { + Ok(Some(mut previous)) => { + previous.alias = None; + previous.updated_at_unix_ms = now; + if let Err(error) = store.write_record_unlocked(&previous) { + warn!( + alias = %alias, + previous_snapshot_id = %existing, + error = %error, + "failed to clear previous snapshot alias metadata" + ); + } + } + Ok(None) => {} + Err(error) => { + warn!( + alias = %alias, + previous_snapshot_id = %existing, + error = %error, + "failed to load previous snapshot alias metadata" + ); } - store.remove_file_if_exists(&alias_path)?; } } - store.write_json(&alias_path, &snapshot_id)?; - store.write_commit_marker(&session.snapshot_id)?; - store.write_committed_record_unlocked(&record)?; Ok(record) }) } else { @@ -123,17 +147,7 @@ impl PosixFsCatalogStore { match write_result { Ok(record) => Ok(record), Err(error) => { - if let Some(alias) = metadata.alias.as_ref() { - let _ = self.with_alias_lock(alias, |store| { - let alias_path = - PosixFsSnapshotArtifactLayout::alias_path(&store.root, alias); - if store.load_alias_target(alias)?.as_ref() == Some(&snapshot_id) { - store.remove_file_if_exists(&alias_path)?; - } - Ok(()) - }); - } - let _ = self.cleanup_uncommitted_snapshot_dir(&session.snapshot_id); + self.rollback_failed_publish(&session.snapshot_id, previous_record.as_ref()); Err(error) } } @@ -164,12 +178,25 @@ impl PosixFsCatalogStore { if let Some(alias) = record.alias.as_ref() { self.with_alias_lock(alias, |store| { - store.ensure_alias_available(alias, &record.id)?; store.write_record_unlocked(&record)?; - store.write_json( - &PosixFsSnapshotArtifactLayout::alias_path(&store.root, alias), - &record.id, - ) + let alias_path = PosixFsSnapshotArtifactLayout::alias_path(&store.root, alias); + match store.load_alias_target(alias)? { + // The alias currently points at a live snapshot. Leave the + // binding untouched so the existing template keeps resolving + // while the new build runs; a successful commit moves the + // alias to the new snapshot (E2B rebuild semantics). + Some(existing) + if existing != record.id + && store.load_record_by_id_unlocked(&existing)?.is_some() => + { + Ok(()) + } + Some(existing) if existing != record.id => { + store.remove_file_if_exists(&alias_path)?; + store.write_json(&alias_path, &record.id) + } + _ => store.write_json(&alias_path, &record.id), + } })?; } else { self.write_record_unlocked(&record)?; @@ -498,6 +525,19 @@ impl PosixFsCatalogStore { self.remove_dir_if_exists(&snapshot_layout.snapshot_dir()) } + fn rollback_failed_publish(&self, id: &SnapshotId, previous_record: Option<&SnapshotRecord>) { + if let Err(error) = self.remove_dir_if_exists(&self.layout(id).snapshot_dir()) { + warn!(snapshot_id = %id, error = %error, "failed to remove snapshot artifacts after publish failure"); + } + let restore_result = match previous_record { + Some(record) => self.write_record_unlocked(record), + None => self.remove_file_if_exists(&self.record_path(id)), + }; + if let Err(error) = restore_result { + warn!(snapshot_id = %id, error = %error, "failed to restore snapshot record after publish failure"); + } + } + fn load_record_by_id_unlocked( &self, id: &SnapshotId, @@ -626,28 +666,6 @@ impl PosixFsCatalogStore { action(self) } - fn ensure_alias_available( - &self, - alias: &SnapshotAlias, - new_id: &SnapshotId, - ) -> RepositoryResult<()> { - let alias_path = PosixFsSnapshotArtifactLayout::alias_path(&self.root, alias); - if let Some(existing) = self.load_alias_target(alias)? { - if &existing == new_id { - return Ok(()); - } - if self.load_record_by_id_unlocked(&existing)?.is_some() { - return Err(RepositoryError::AliasConflict { - alias: alias.to_string(), - existing, - new_id: new_id.clone(), - }); - } - self.remove_file_if_exists(&alias_path)?; - } - Ok(()) - } - fn write_record_unlocked(&self, record: &SnapshotRecord) -> RepositoryResult<()> { self.write_json(&self.record_path(&record.id), record) } diff --git a/src/snapshot/repository/backends/posixfs/mod.rs b/src/snapshot/repository/backends/posixfs/mod.rs index 09ce72af..f61204dc 100644 --- a/src/snapshot/repository/backends/posixfs/mod.rs +++ b/src/snapshot/repository/backends/posixfs/mod.rs @@ -1,5 +1,6 @@ mod artifacts; mod backend; +mod build_files; mod catalog; mod layout; mod runtime; diff --git a/src/snapshot/repository/build_files.rs b/src/snapshot/repository/build_files.rs new file mode 100644 index 00000000..71a47841 --- /dev/null +++ b/src/snapshot/repository/build_files.rs @@ -0,0 +1,164 @@ +use std::path::{Path, PathBuf}; + +use async_trait::async_trait; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use base64::Engine; +use serde::{Deserialize, Serialize}; + +use super::errors::RepositoryResult; + +/// Number of random bytes in an upload bearer token. +pub const UPLOAD_TOKEN_LEN: usize = 32; + +/// Durable authorization record for one build-context upload URL. +/// +/// Grants live in the same shared repository as build archives. That makes a +/// URL issued by one node verifiable by any other node without coordinating a +/// deployment-wide in-memory signing secret. +#[derive(Debug, Deserialize, Serialize)] +pub struct TemplateBuildUploadGrant { + pub template_id: String, + pub hash: String, + pub expires_unix: i64, +} + +impl TemplateBuildUploadGrant { + pub fn new(template_id: &str, hash: &str, expires_unix: i64) -> Self { + Self { + template_id: template_id.to_string(), + hash: hash.to_string(), + expires_unix, + } + } + + pub fn authorizes( + &self, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> bool { + now_unix <= expires_unix + && self.expires_unix == expires_unix + && self.template_id == template_id + && self.hash == hash + } +} + +/// Durable store for template build-context archives. +/// +/// The E2B SDK resolves every `COPY` step through +/// `GET /templates/{templateID}/files/{hash}` and then `PUT`s a tar archive of +/// the matching context files to the returned URL. This store owns those +/// archives, addressed by the SDK-computed content hash, so that: +/// +/// - any node can answer the upload-link request (`exists`), +/// - any node can accept the upload (`import`), and +/// - the node that runs the build can read the archive back (`materialize`). +/// +/// Implementations must place the archives in storage shared by all nodes of +/// the deployment, mirroring the visibility rules of committed snapshots. +#[async_trait] +pub trait TemplateBuildFileStore: Send + Sync { + /// Returns whether an archive for `hash` is already stored. + async fn exists(&self, hash: &str) -> RepositoryResult; + + /// Imports a fully written local file as the archive for `hash`. + /// + /// Implementations must publish atomically: concurrent readers never + /// observe a partially imported archive. Re-importing an existing hash + /// replaces the stored archive. + async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; + + /// Materializes the archive for `hash` as a node-local file. + /// + /// `scratch_dir` is a caller-owned directory the implementation may use + /// for downloads; implementations backed by a shared filesystem may return + /// the shared path directly. Callers must treat the returned file as + /// read-only. Returns `None` when no archive is stored for `hash`. + async fn materialize( + &self, + hash: &str, + scratch_dir: &Path, + ) -> RepositoryResult>; + + /// Creates a durable bearer grant for one upload URL and returns its + /// URL-safe token. + async fn create_upload_grant( + &self, + template_id: &str, + hash: &str, + expires_unix: i64, + ) -> RepositoryResult; + + /// Returns whether a durable bearer grant authorizes this upload. + async fn validate_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult; +} + +/// Returns whether `hash` is acceptable as a build-file content hash. +/// +/// The E2B SDK sends a lowercase hex SHA-256, but the value is treated as an +/// opaque cache key; this only enforces a path- and URL-safe shape. +pub fn is_valid_build_files_hash(hash: &str) -> bool { + (16..=128).contains(&hash.len()) && hash.bytes().all(|b| b.is_ascii_hexdigit()) +} + +/// Generates a cryptographically random URL-safe upload bearer token. +pub fn generate_upload_token() -> String { + let mut token = [0u8; UPLOAD_TOKEN_LEN]; + rand::fill(&mut token); + URL_SAFE_NO_PAD.encode(token) +} + +/// Returns whether `token` has the exact shape generated for upload grants. +pub fn is_valid_upload_token(token: &str) -> bool { + URL_SAFE_NO_PAD + .decode(token) + .is_ok_and(|decoded| decoded.len() == UPLOAD_TOKEN_LEN) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hash_validation_accepts_sha256_hex() { + assert!(is_valid_build_files_hash( + "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" + )); + assert!(is_valid_build_files_hash("ABCDEF0123456789")); + } + + #[test] + fn hash_validation_rejects_path_unsafe_values() { + assert!(!is_valid_build_files_hash("")); + assert!(!is_valid_build_files_hash("short")); + assert!(!is_valid_build_files_hash("../../../../etc/passwd")); + assert!(!is_valid_build_files_hash("deadbeef/deadbeef")); + assert!(!is_valid_build_files_hash(&"a".repeat(129))); + } + + #[test] + fn upload_token_has_expected_shape() { + let token = generate_upload_token(); + assert!(is_valid_upload_token(&token)); + assert!(!is_valid_upload_token("not-a-valid-token")); + } + + #[test] + fn upload_grant_is_bound_to_request_and_expiry() { + let grant = TemplateBuildUploadGrant::new("tmpl", "aabbccddeeff0011", 1000); + assert!(grant.authorizes("tmpl", "aabbccddeeff0011", 1000, 999)); + assert!(!grant.authorizes("tmpl", "aabbccddeeff0011", 1000, 1001)); + assert!(!grant.authorizes("other", "aabbccddeeff0011", 1000, 999)); + assert!(!grant.authorizes("tmpl", "aabbccddeeff0012", 1000, 999)); + assert!(!grant.authorizes("tmpl", "aabbccddeeff0011", 2000, 999)); + } +} diff --git a/src/snapshot/repository/interfaces.rs b/src/snapshot/repository/interfaces.rs index 16c387f4..604b07a6 100644 --- a/src/snapshot/repository/interfaces.rs +++ b/src/snapshot/repository/interfaces.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use async_trait::async_trait; +use super::build_files::TemplateBuildFileStore; use super::errors::RepositoryResult; use crate::sandbox::FirecrackerSnapshotManifest; use crate::snapshot::types::{ @@ -173,6 +174,15 @@ pub trait SnapshotRepository: Send + Sync { id: &SnapshotId, reason: TemplateBuildErrorReason, ) -> RepositoryResult<()>; + + /// Returns the shared store for template build-context archives. + /// + /// Returns `None` when this backend does not support build-context + /// uploads; the template files API then reports the capability as + /// unavailable instead of failing at build time. + fn template_build_files(&self) -> Option> { + None + } } #[async_trait] diff --git a/src/snapshot/repository/mod.rs b/src/snapshot/repository/mod.rs index 788c94e8..aa45e140 100644 --- a/src/snapshot/repository/mod.rs +++ b/src/snapshot/repository/mod.rs @@ -1,6 +1,8 @@ pub mod backends; +pub mod build_files; pub mod errors; pub mod interfaces; +pub use build_files::TemplateBuildFileStore; pub use errors::{RepositoryError, RepositoryResult}; pub use interfaces::{SnapshotListFilter, SnapshotRepository, SnapshotRuntimeResolver}; diff --git a/src/template/build_spec.rs b/src/template/build_spec.rs index 8e23e18b..1faf9a48 100644 --- a/src/template/build_spec.rs +++ b/src/template/build_spec.rs @@ -25,13 +25,41 @@ pub(crate) struct TemplateBuildStep { #[derive(Clone, Debug)] pub(crate) enum TemplateBuildStepKind { - Run { cmd: String }, - Env { key: String, value: String }, - Workdir { path: PathBuf }, - User { value: String }, - ExposedPort { port: String }, - Volume { path: String }, - Label { key: String, value: String }, + Run { + cmd: String, + }, + Env { + key: String, + value: String, + }, + Workdir { + path: PathBuf, + }, + User { + value: String, + }, + ExposedPort { + port: String, + }, + Volume { + path: String, + }, + Label { + key: String, + value: String, + }, + /// Copies files from an uploaded build-context archive into the rootfs. + /// + /// `files_hash` addresses the archive in the repository's template + /// build-file store; `user` and `mode` mirror Docker's `--chown` / + /// `--chmod` flags. + Copy { + src: String, + dest: String, + files_hash: String, + user: Option, + mode: Option, + }, } impl TemplateBuildStep { @@ -84,6 +112,24 @@ impl TemplateBuildStep { }, } } + + pub(crate) fn copy( + src: impl Into, + dest: impl Into, + files_hash: impl Into, + user: Option, + mode: Option, + ) -> Self { + Self { + kind: TemplateBuildStepKind::Copy { + src: src.into(), + dest: dest.into(), + files_hash: files_hash.into(), + user, + mode, + }, + } + } } #[derive(Clone, Debug, Default)] @@ -167,6 +213,34 @@ impl TemplateBuildSpec { self } + /// Appends a build step copying files from an uploaded build-context + /// archive (addressed by `files_hash`) into the rootfs. + pub fn copy( + mut self, + src: impl Into, + dest: impl Into, + files_hash: impl Into, + user: Option, + mode: Option, + ) -> Self { + self.steps + .push(TemplateBuildStep::copy(src, dest, files_hash, user, mode)); + self + } + + /// Returns the distinct build-context archive hashes referenced by COPY steps. + pub(crate) fn referenced_build_file_hashes(steps: &[TemplateBuildStep]) -> Vec { + let mut hashes: Vec = Vec::new(); + for step in steps { + if let TemplateBuildStepKind::Copy { files_hash, .. } = &step.kind { + if !hashes.iter().any(|existing| existing == files_hash) { + hashes.push(files_hash.clone()); + } + } + } + hashes + } + /// Appends an `apt-get install` build step for the provided packages. pub fn apt(mut self, packages: I) -> Self where diff --git a/src/template/builder.rs b/src/template/builder.rs index c934e154..54a2d290 100644 --- a/src/template/builder.rs +++ b/src/template/builder.rs @@ -100,9 +100,12 @@ impl TemplateBuilder { async fn execute_and_publish( &self, snapshot_manager: &SnapshotManager, - context: TemplateBuildContext, + mut context: TemplateBuildContext, operation: &'static str, ) -> TemplatePipelineResult { + context.build_archives = + Self::materialize_build_archives(snapshot_manager, &context).await?; + info!("executing template build"); let build_execution = match TemplateBuildRunner::new().execute(&context) { Ok(execution) => execution, @@ -151,6 +154,55 @@ impl TemplateBuilder { } impl TemplateBuilder { + /// Fetches every build-context archive referenced by COPY steps into + /// node-local files before the build sandbox starts. + async fn materialize_build_archives( + snapshot_manager: &SnapshotManager, + context: &TemplateBuildContext, + ) -> TemplatePipelineResult> { + let hashes = TemplateBuildSpec::referenced_build_file_hashes(&context.steps); + if hashes.is_empty() { + return Ok(std::collections::HashMap::new()); + } + + let Some(store) = snapshot_manager.template_build_files() else { + return Err(TemplateBuildError::invalid_input( + "the configured snapshot backend does not support the build-context uploads required by COPY steps", + ) + .into()); + }; + + let scratch = context.local_dir().join("build-archives"); + tokio::fs::create_dir_all(&scratch).await.map_err(|error| { + TemplateBuildError::with_source("create build archive scratch dir", error) + })?; + + let mut archives = std::collections::HashMap::new(); + for hash in hashes { + match store.materialize(&hash, &scratch).await { + Ok(Some(path)) => { + archives.insert(hash, path); + } + Ok(None) => { + return Err(TemplateBuildError::invalid_input(format!( + "build context archive '{hash}' has not been uploaded; request an upload \ + link via GET /templates/{{templateID}}/files/{hash} and upload the \ + archive before starting the build" + )) + .into()); + } + Err(error) => { + return Err(TemplateBuildError::with_source( + format!("fetch build context archive '{hash}'"), + error, + ) + .into()); + } + } + } + Ok(archives) + } + fn build_failure_reason(error: &AnyhowError) -> TemplateBuildErrorReason { error .chain() @@ -209,6 +261,7 @@ impl TemplateBuilder { resources, workspace, steps: spec.steps().to_vec(), + build_archives: std::collections::HashMap::new(), base, cpu_config_json: self.current_cpu_config(), }) @@ -261,6 +314,7 @@ impl TemplateBuilder { resources, workspace, steps: spec.steps().to_vec(), + build_archives: std::collections::HashMap::new(), base: TemplateBuildBase::Snapshot { base_snapshot: Box::new(base_snapshot.clone()), }, diff --git a/src/template/copy_plan.rs b/src/template/copy_plan.rs new file mode 100644 index 00000000..5d4b3436 --- /dev/null +++ b/src/template/copy_plan.rs @@ -0,0 +1,697 @@ +//! Host-side planning for template `COPY` steps. +//! +//! The E2B SDK uploads one tar archive per `COPY` instruction whose entry +//! paths are relative to the build context (the glob in `src` is already +//! resolved by the SDK). This module rewrites that archive so every entry +//! carries its final absolute guest path per Docker `COPY` semantics; the +//! build sandbox then only needs a single `tar -xpf archive -C /`. +//! +//! Ownership is normalized to root:root (Docker's `COPY` default — the SDK +//! archive carries the uploader's local uids); an explicit `--chown` user is +//! applied afterwards with `chown -R` on the created roots. + +use std::fs::File; +use std::io::{BufReader, Read, Seek, SeekFrom, Write}; +use std::path::Path; + +use anyhow::{bail, Context, Result}; + +/// Summary of a rewritten copy archive. +#[derive(Debug)] +pub(crate) struct CopyPlan { + /// Absolute guest paths of the top-level items this copy creates, used + /// for the optional post-extract `chown -R`. + pub(crate) created_roots: Vec, + /// Number of file/dir/symlink entries written to the rewritten archive. + pub(crate) entry_count: usize, +} + +/// One entry read from the SDK context archive. +struct SourceEntry { + /// Normalized context-relative path ("dir/file.txt"). + path: String, + header: tar::Header, + link_name: Option, + /// Byte range of the entry data within the decompressed stream is not + /// seekable, so file contents are buffered per entry during rewrite. + data: Vec, +} + +fn is_glob_pattern(src: &str) -> bool { + src.contains(['*', '?', '[']) +} + +/// Minimal fnmatch-style matcher covering `*`, `?` and `[...]` (no `**`), +/// mirroring the Python `glob` patterns the SDK resolves client-side. +fn glob_match(pattern: &str, value: &str) -> bool { + fn inner(pattern: &[char], value: &[char]) -> bool { + match pattern.split_first() { + None => value.is_empty(), + Some(('*', rest)) => (0..=value.len()).any(|skip| inner(rest, &value[skip..])), + Some(('?', rest)) => !value.is_empty() && inner(rest, &value[1..]), + Some(('[', rest)) => { + let Some(end) = rest.iter().position(|&c| c == ']') else { + // No closing bracket: treat '[' as a literal character. + return !value.is_empty() && value[0] == '[' && inner(rest, &value[1..]); + }; + let (class, after) = rest.split_at(end); + let after = &after[1..]; + let Some(&first) = value.first() else { + return false; + }; + let (negated, class) = match class.first() { + Some('!') | Some('^') => (true, &class[1..]), + _ => (false, class), + }; + let mut matched = false; + let mut i = 0; + while i < class.len() { + if i + 2 < class.len() && class[i + 1] == '-' { + if class[i] <= first && first <= class[i + 2] { + matched = true; + } + i += 3; + } else { + if class[i] == first { + matched = true; + } + i += 1; + } + } + if matched != negated { + inner(after, &value[1..]) + } else { + false + } + } + Some((&c, rest)) => !value.is_empty() && value[0] == c && inner(rest, &value[1..]), + } + } + let pattern: Vec = pattern.chars().collect(); + let value: Vec = value.chars().collect(); + inner(&pattern, &value) +} + +/// Normalizes a context-relative source pattern ("./a/b/" -> "a/b"). +fn normalize_src(src: &str) -> String { + let mut src = src.trim(); + while let Some(stripped) = src.strip_prefix("./") { + src = stripped; + } + src.trim_end_matches('/').to_string() +} + +/// Joins and lexically normalizes an absolute guest destination path. +fn resolve_dest(dest: &str, workdir: &str) -> Result { + let joined = if dest.starts_with('/') { + dest.to_string() + } else { + let workdir = if workdir.trim().is_empty() { + "/" + } else { + workdir + }; + if !workdir.starts_with('/') { + bail!("workdir '{workdir}' must be absolute to resolve relative COPY destination"); + } + format!("{}/{}", workdir.trim_end_matches('/'), dest) + }; + + let mut parts: Vec<&str> = Vec::new(); + for part in joined.split('/') { + match part { + "" | "." => {} + ".." => { + if parts.pop().is_none() { + bail!("COPY destination '{dest}' escapes the filesystem root"); + } + } + part => parts.push(part), + } + } + Ok(format!("/{}", parts.join("/"))) +} + +fn join_abs(base: &str, rel: &str) -> String { + if rel.is_empty() { + base.to_string() + } else if base == "/" { + format!("/{rel}") + } else { + format!("{base}/{rel}") + } +} + +fn base_name(path: &str) -> &str { + path.rsplit('/').next().unwrap_or(path) +} + +/// Normalizes one archive entry path and rejects escapes. +fn normalize_entry_path(raw: &Path) -> Result { + let mut parts: Vec = Vec::new(); + for component in raw.components() { + match component { + std::path::Component::Normal(part) => { + parts.push(part.to_string_lossy().into_owned()); + } + std::path::Component::CurDir => {} + other => bail!( + "unsupported path component {:?} in archive entry '{}'", + other, + raw.display() + ), + } + } + if parts.is_empty() { + bail!("empty path in archive entry"); + } + Ok(parts.join("/")) +} + +fn read_source_entries(source_tar: &Path) -> Result> { + let mut file = File::open(source_tar) + .with_context(|| format!("open build context archive '{}'", source_tar.display()))?; + let mut magic = [0u8; 2]; + let gzip = match file.read(&mut magic) { + Ok(2) => magic == [0x1f, 0x8b], + _ => false, + }; + file.seek(SeekFrom::Start(0)) + .context("rewind build context archive")?; + + let reader: Box = if gzip { + Box::new(flate2::read::GzDecoder::new(BufReader::new(file))) + } else { + Box::new(BufReader::new(file)) + }; + + let mut archive = tar::Archive::new(reader); + let mut entries = Vec::new(); + for entry in archive + .entries() + .context("read build context archive entries")? + { + let mut entry = entry.context("read build context archive entry")?; + let entry_type = entry.header().entry_type(); + match entry_type { + tar::EntryType::Regular + | tar::EntryType::Directory + | tar::EntryType::Symlink + | tar::EntryType::GNUSparse => {} + // Metadata-only companion entries (long names, pax headers) are + // consumed by the tar crate itself and never surface here. + other => bail!("unsupported entry type {other:?} in build context archive"), + } + + let path = normalize_entry_path(&entry.path().context("entry path")?)?; + let link_name = entry + .link_name() + .context("entry link name")? + .map(|l| l.into_owned()); + let mut data = Vec::new(); + if entry_type == tar::EntryType::Regular || entry_type == tar::EntryType::GNUSparse { + entry.read_to_end(&mut data).context("entry contents")?; + } + entries.push(SourceEntry { + path, + header: entry.header().clone(), + link_name, + data, + }); + } + if entries.is_empty() { + bail!("build context archive contains no files"); + } + Ok(entries) +} + +/// Computes the final absolute guest path for every entry. +/// +/// Returns `(mappings, created_roots)` where `mappings[i]` matches +/// `entries[i]`. +fn map_entries( + entries: &[SourceEntry], + src: &str, + dest_raw: &str, + workdir: &str, +) -> Result<(Vec, Vec)> { + let src = normalize_src(src); + let dest_is_dir_hint = dest_raw.ends_with('/') + || dest_raw.ends_with("/.") + || dest_raw == "." + || dest_raw.is_empty(); + let dest = resolve_dest(if dest_raw.is_empty() { "." } else { dest_raw }, workdir)?; + + let mut mapped = Vec::with_capacity(entries.len()); + let mut roots: Vec = Vec::new(); + let mut push_root = |root: String| { + if !roots.contains(&root) { + roots.push(root); + } + }; + + let copy_whole_context = src.is_empty() || src == "."; + let single_file_src = !copy_whole_context + && !is_glob_pattern(&src) + && entries.len() == 1 + && entries[0].path == src + && entries[0].header.entry_type() != tar::EntryType::Directory; + + if single_file_src { + let target = if dest_is_dir_hint { + join_abs(&dest, base_name(&src)) + } else { + dest.clone() + }; + push_root(target.clone()); + mapped.push(target); + return Ok((mapped, roots)); + } + + if copy_whole_context || !is_glob_pattern(&src) { + // Directory source: Docker copies the directory *contents* into dest. + for entry in entries { + let rel = if copy_whole_context { + entry.path.as_str() + } else if entry.path == src { + "" + } else if let Some(rel) = entry.path.strip_prefix(&format!("{src}/")) { + rel + } else { + bail!( + "archive entry '{}' does not belong to COPY source '{}'", + entry.path, + src + ); + }; + mapped.push(join_abs(&dest, rel)); + } + push_root(dest.clone()); + return Ok((mapped, roots)); + } + + // Glob source: every matched top-level item lands inside dest. Matched + // files keep their base name; matched directories contribute their + // contents (Docker treats each matched directory like a directory source). + for entry in entries { + let mut components = entry.path.split('/'); + let mut prefix = String::new(); + let mut matched_root: Option = None; + for component in components.by_ref() { + if prefix.is_empty() { + prefix.push_str(component); + } else { + prefix.push('/'); + prefix.push_str(component); + } + if glob_match(&src, &prefix) { + matched_root = Some(prefix.clone()); + break; + } + } + let Some(root) = matched_root else { + bail!( + "archive entry '{}' does not match COPY source pattern '{}'", + entry.path, + src + ); + }; + let rel = entry + .path + .strip_prefix(&root) + .map(|rest| rest.trim_start_matches('/')) + .unwrap_or(""); + let target = if rel.is_empty() && entry.header.entry_type() != tar::EntryType::Directory { + let target = join_abs(&dest, base_name(&root)); + push_root(target.clone()); + target + } else { + push_root(dest.clone()); + join_abs(&dest, rel) + }; + mapped.push(target); + } + Ok((mapped, roots)) +} + +/// Rewrites the SDK context archive into `output` with final absolute guest +/// paths, root ownership, and the optional mode override applied. +pub(crate) fn plan_copy_archive( + source_tar: &Path, + src: &str, + dest: &str, + workdir: &str, + mode_override: Option, + output: &Path, +) -> Result { + let entries = read_source_entries(source_tar)?; + let (mapped, created_roots) = map_entries(&entries, src, dest, workdir)?; + + let out_file = File::create(output) + .with_context(|| format!("create rewritten copy archive '{}'", output.display()))?; + let mut builder = tar::Builder::new(out_file); + let mut entry_count = 0usize; + + for (entry, target) in entries.iter().zip(mapped.iter()) { + let relative = target.trim_start_matches('/'); + if relative.is_empty() { + // The destination root itself ("/"); parents always exist. + continue; + } + + let mut header = entry.header.clone(); + header.set_uid(0); + header.set_gid(0); + let _ = header.set_username(""); + let _ = header.set_groupname(""); + if let Some(mode) = mode_override { + header.set_mode(mode); + } + + match entry.header.entry_type() { + tar::EntryType::Directory => { + header.set_size(0); + builder + .append_data(&mut header, format!("{relative}/"), std::io::empty()) + .with_context(|| format!("write directory entry '{target}'"))?; + } + tar::EntryType::Symlink => { + let link = entry + .link_name + .as_ref() + .context("symlink entry is missing its target")?; + header.set_size(0); + builder + .append_link(&mut header, relative, link) + .with_context(|| format!("write symlink entry '{target}'"))?; + } + _ => { + header.set_size(entry.data.len() as u64); + builder + .append_data(&mut header, relative, entry.data.as_slice()) + .with_context(|| format!("write file entry '{target}'"))?; + } + } + entry_count += 1; + } + + let mut out_file = builder.into_inner().context("finish rewritten archive")?; + out_file.flush().context("flush rewritten archive")?; + + Ok(CopyPlan { + created_roots, + entry_count, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::BTreeMap; + use tempfile::TempDir; + + fn build_source_tar(dir: &Path, entries: &[(&str, Option<&str>)]) -> std::path::PathBuf { + // (path, Some(contents)) = file, (path, None) = directory + let tar_path = dir.join("source.tar"); + let file = File::create(&tar_path).expect("create tar"); + let mut builder = tar::Builder::new(file); + for (path, contents) in entries { + let mut header = tar::Header::new_gnu(); + header.set_uid(501); + header.set_gid(20); + match contents { + Some(data) => { + header.set_entry_type(tar::EntryType::Regular); + header.set_mode(0o644); + header.set_size(data.len() as u64); + builder + .append_data(&mut header, path, data.as_bytes()) + .expect("append file"); + } + None => { + header.set_entry_type(tar::EntryType::Directory); + header.set_mode(0o755); + header.set_size(0); + builder + .append_data(&mut header, format!("{path}/"), std::io::empty()) + .expect("append dir"); + } + } + } + builder.finish().expect("finish tar"); + tar_path + } + + fn rewritten_entries(path: &Path) -> BTreeMap { + let mut archive = tar::Archive::new(File::open(path).expect("open rewritten")); + let mut out = BTreeMap::new(); + for entry in archive.entries().expect("entries") { + let mut entry = entry.expect("entry"); + let path = entry.path().expect("path").to_string_lossy().into_owned(); + let kind = entry.header().entry_type(); + let uid = entry.header().uid().expect("uid"); + let mode = entry.header().mode().expect("mode"); + let mut contents = String::new(); + entry.read_to_string(&mut contents).expect("read"); + out.insert(path, (kind, uid, mode, contents)); + } + out + } + + #[test] + fn single_file_to_absolute_file_dest() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("hello.txt", Some("hello\n"))]); + let out = dir.path().join("out.tar"); + + let plan = + plan_copy_archive(&tar, "hello.txt", "/hello.txt", "/", None, &out).expect("plan"); + + assert_eq!(plan.entry_count, 1); + assert_eq!(plan.created_roots, vec!["/hello.txt".to_string()]); + let entries = rewritten_entries(&out); + let (kind, uid, _, contents) = &entries["hello.txt"]; + assert_eq!(*kind, tar::EntryType::Regular); + assert_eq!(*uid, 0, "ownership must be normalized to root"); + assert_eq!(contents, "hello\n"); + } + + #[test] + fn single_file_to_directory_dest() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("requirements.txt", Some("e2b\n"))]); + let out = dir.path().join("out.tar"); + + let plan = plan_copy_archive(&tar, "requirements.txt", "/home/user/", "/", None, &out) + .expect("plan"); + + assert_eq!( + plan.created_roots, + vec!["/home/user/requirements.txt".to_string()] + ); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("home/user/requirements.txt")); + } + + #[test] + fn relative_dest_resolves_against_workdir() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("config.py", Some("x = 1\n"))]); + let out = dir.path().join("out.tar"); + + let plan = + plan_copy_archive(&tar, "config.py", "conf/app.py", "/srv", None, &out).expect("plan"); + + assert_eq!(plan.created_roots, vec!["/srv/conf/app.py".to_string()]); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("srv/conf/app.py")); + } + + #[test] + fn directory_source_copies_contents_into_dest() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar( + dir.path(), + &[ + ("app", None), + ("app/main.py", Some("print()\n")), + ("app/sub", None), + ("app/sub/util.py", Some("pass\n")), + ], + ); + let out = dir.path().join("out.tar"); + + let plan = plan_copy_archive(&tar, "app", "/opt/service", "/", None, &out).expect("plan"); + + assert_eq!(plan.created_roots, vec!["/opt/service".to_string()]); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("opt/service/")); + assert!(entries.contains_key("opt/service/main.py")); + assert!(entries.contains_key("opt/service/sub/")); + assert!(entries.contains_key("opt/service/sub/util.py")); + } + + #[test] + fn whole_context_source_copies_everything() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar( + dir.path(), + &[ + ("a.txt", Some("a")), + ("sub", None), + ("sub/b.txt", Some("b")), + ], + ); + let out = dir.path().join("out.tar"); + + let plan = plan_copy_archive(&tar, ".", "/workspace", "/", None, &out).expect("plan"); + + assert_eq!(plan.created_roots, vec!["/workspace".to_string()]); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("workspace/a.txt")); + assert!(entries.contains_key("workspace/sub/b.txt")); + } + + #[test] + fn glob_source_places_matches_by_base_name() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar( + dir.path(), + &[("one.txt", Some("1")), ("two.txt", Some("2"))], + ); + let out = dir.path().join("out.tar"); + + let plan = plan_copy_archive(&tar, "*.txt", "/data/", "/", None, &out).expect("plan"); + + assert_eq!(plan.entry_count, 2); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("data/one.txt")); + assert!(entries.contains_key("data/two.txt")); + assert_eq!( + plan.created_roots, + vec!["/data/one.txt".to_string(), "/data/two.txt".to_string()] + ); + } + + #[test] + fn glob_matching_directory_copies_its_contents() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar( + dir.path(), + &[ + ("pkg-a", None), + ("pkg-a/lib.py", Some("a")), + ("pkg-b", None), + ("pkg-b/lib.py", Some("b")), + ], + ); + let out = dir.path().join("out.tar"); + + let plan = plan_copy_archive(&tar, "pkg-*", "/opt/pkgs", "/", None, &out).expect("plan"); + + let entries = rewritten_entries(&out); + // Docker merges contents of every matched directory into dest; the + // second lib.py overwrites the first at extract time. + assert!(entries.contains_key("opt/pkgs/lib.py")); + assert_eq!(plan.created_roots, vec!["/opt/pkgs".to_string()]); + } + + #[test] + fn mode_override_applies_to_entries() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("run.sh", Some("#!/bin/sh\n"))]); + let out = dir.path().join("out.tar"); + + plan_copy_archive( + &tar, + "run.sh", + "/usr/local/bin/run.sh", + "/", + Some(0o755), + &out, + ) + .expect("plan"); + + let entries = rewritten_entries(&out); + let (_, _, mode, _) = &entries["usr/local/bin/run.sh"]; + assert_eq!(*mode, 0o755); + } + + #[test] + fn gzip_archives_are_accepted() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("hello.txt", Some("hi"))]); + let gz_path = dir.path().join("source.tar.gz"); + let mut encoder = flate2::write::GzEncoder::new( + File::create(&gz_path).expect("create gz"), + flate2::Compression::fast(), + ); + std::io::copy(&mut File::open(&tar).expect("open tar"), &mut encoder).expect("compress"); + encoder.finish().expect("finish gz"); + let out = dir.path().join("out.tar"); + + let plan = + plan_copy_archive(&gz_path, "hello.txt", "/hello.txt", "/", None, &out).expect("plan"); + assert_eq!(plan.entry_count, 1); + } + + #[test] + fn rejects_entries_escaping_the_root() { + let dir = TempDir::new().expect("tempdir"); + let tar_path = dir.path().join("evil.tar"); + let file = File::create(&tar_path).expect("create tar"); + let mut builder = tar::Builder::new(file); + // `append_data` refuses to write `..` paths, so craft the header + // manually the way a hostile client would. + let mut header = tar::Header::new_gnu(); + let evil_path = b"../../etc/passwd"; + header.as_gnu_mut().expect("gnu header").name[..evil_path.len()].copy_from_slice(evil_path); + header.set_entry_type(tar::EntryType::Regular); + header.set_mode(0o644); + header.set_size(4); + header.set_cksum(); + builder + .append(&header, "pwn\n".as_bytes()) + .expect("append raw entry"); + builder.finish().expect("finish"); + let out = dir.path().join("out.tar"); + + let err = plan_copy_archive(&tar_path, "passwd", "/tmp/x", "/", None, &out) + .expect_err("path escape must fail"); + assert!(err.to_string().contains("unsupported path component")); + } + + #[test] + fn rejects_dest_escaping_the_root() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("a.txt", Some("a"))]); + let out = dir.path().join("out.tar"); + + let err = plan_copy_archive(&tar, "a.txt", "../../x", "/", None, &out) + .expect_err("dest escape must fail"); + assert!(err.to_string().contains("escapes the filesystem root")); + } + + #[test] + fn rejects_empty_archive() { + let dir = TempDir::new().expect("tempdir"); + let tar_path = dir.path().join("empty.tar"); + let file = File::create(&tar_path).expect("create tar"); + tar::Builder::new(file).finish().expect("finish"); + let out = dir.path().join("out.tar"); + + let err = plan_copy_archive(&tar_path, "x", "/x", "/", None, &out) + .expect_err("empty archive must fail"); + assert!(err.to_string().contains("no files")); + } + + #[test] + fn glob_match_basics() { + assert!(glob_match("*.txt", "a.txt")); + assert!(!glob_match("*.txt", "a.txt.bak")); + assert!(glob_match("data?", "data1")); + assert!(glob_match("[ab]*", "b12")); + assert!(!glob_match("[!ab]*", "b12")); + assert!(glob_match("pkg-*", "pkg-a")); + } +} diff --git a/src/template/mod.rs b/src/template/mod.rs index 57800f46..53aaabd6 100644 --- a/src/template/mod.rs +++ b/src/template/mod.rs @@ -1,5 +1,6 @@ mod build_spec; mod builder; +mod copy_plan; mod errors; mod runner; mod step_executor; diff --git a/src/template/runner.rs b/src/template/runner.rs index 1879f11e..ff7c5dde 100644 --- a/src/template/runner.rs +++ b/src/template/runner.rs @@ -64,6 +64,9 @@ pub(crate) struct TemplateBuildContext { // Keep the tempdir owner alive for the duration of build + publish. pub workspace: TempDir, pub steps: Vec, + /// Node-local archives for COPY steps, keyed by files hash. Populated by + /// the builder before execution. + pub build_archives: std::collections::HashMap, pub base: TemplateBuildBase, pub cpu_config_json: Option, } @@ -197,6 +200,7 @@ impl TemplateBuildRunner { let worker_span = tracing::debug_span!("template_build_sandbox", sandbox_id = %sandbox_id); let step_executor = self.step_executor.clone(); let steps = context.steps.clone(); + let build_archives = context.build_archives.clone(); let output_dir = context.local_dir().to_path_buf(); let initial_context = context.initial_context.clone(); let startup = context.startup.clone(); @@ -220,7 +224,7 @@ impl TemplateBuildRunner { debug!("template build sandbox started"); let build_context = step_executor - .execute(&sandbox, &steps, initial_context) + .execute(&sandbox, &steps, initial_context, &build_archives) .await?; let startup = prepare_startup(startup, override_startup, &build_context); run_startup_commands(&sandbox, startup.as_ref()).await?; diff --git a/src/template/step_executor.rs b/src/template/step_executor.rs index a920f552..b74e93f6 100644 --- a/src/template/step_executor.rs +++ b/src/template/step_executor.rs @@ -1,13 +1,39 @@ use std::collections::HashMap; +use std::path::PathBuf; use anyhow::{Context, Result}; +use shell_util::shell_quote; use tracing::debug; use super::build_spec::{TemplateBuildStep, TemplateBuildStepKind}; +use super::copy_plan::plan_copy_archive; use super::errors::{command_output_suffix, TemplateBuildFailure}; use crate::sandbox::{ProcessOpts, SandboxExecutor}; use crate::snapshot::CommandContext; +/// Validates a Docker-style `--chown` value (`user`, `uid`, `user:group`). +/// +/// The value ends up in a shell command inside the build sandbox (quoted), so +/// this stays conservative rather than mirroring every libc name rule. +fn is_valid_chown_spec(spec: &str) -> bool { + fn valid_part(part: &str) -> bool { + !part.is_empty() + && part + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'_' | b'-' | b'.')) + } + let mut parts = spec.split(':'); + let (user, group, extra) = (parts.next(), parts.next(), parts.next()); + if extra.is_some() { + return false; + } + match (user, group) { + (Some(user), None) => valid_part(user), + (Some(user), Some(group)) => valid_part(user) && valid_part(group), + _ => false, + } +} + #[derive(Clone, Debug, Default)] pub(crate) struct TemplateStepExecutor; @@ -17,7 +43,7 @@ impl TemplateStepExecutor { } #[tracing::instrument( - skip(self, sandbox, steps, initial_context), + skip(self, sandbox, steps, initial_context, build_archives), fields(step_count = steps.len()) )] pub(crate) async fn execute( @@ -25,6 +51,7 @@ impl TemplateStepExecutor { sandbox: &impl SandboxExecutor, steps: &[TemplateBuildStep], initial_context: CommandContext, + build_archives: &HashMap, ) -> Result { let mut context = initial_context; @@ -63,6 +90,25 @@ impl TemplateStepExecutor { self.run_step(sandbox, &context.workdir, &context.env_vars, cmd) .await?; } + TemplateBuildStepKind::Copy { + src, + dest, + files_hash, + user, + mode, + } => { + self.copy_step( + sandbox, + &context, + build_archives, + src, + dest, + files_hash, + user.as_deref(), + *mode, + ) + .await?; + } } } debug!("template build steps completed"); @@ -70,6 +116,89 @@ impl TemplateStepExecutor { Ok(context) } + /// Applies one COPY step: rewrites the uploaded context archive to final + /// absolute guest paths on the host, streams it into the sandbox via + /// envd, and extracts it at `/` inside the guest. + #[allow(clippy::too_many_arguments)] + async fn copy_step( + &self, + sandbox: &impl SandboxExecutor, + context: &CommandContext, + build_archives: &HashMap, + src: &str, + dest: &str, + files_hash: &str, + user: Option<&str>, + mode: Option, + ) -> Result<()> { + let step_label = format!("COPY {src} {dest}"); + let with_step = |message: String| TemplateBuildFailure::with_step(message, &step_label); + + let archive = build_archives.get(files_hash).ok_or_else(|| { + with_step(format!( + "build step failed: build context archive '{files_hash}' has not been uploaded" + )) + })?; + + if let Some(user) = user { + if !is_valid_chown_spec(user) { + return Err( + with_step(format!("build step failed: invalid COPY user '{user}'")).into(), + ); + } + } + + let rewritten = tempfile::Builder::new() + .prefix("agentenv-copy-") + .suffix(".tar") + .tempfile() + .context("create rewritten copy archive")?; + let plan = plan_copy_archive(archive, src, dest, &context.workdir, mode, rewritten.path()) + .map_err(|error| with_step(format!("build step failed: {error:#}")))?; + debug!( + files_hash, + entries = plan.entry_count, + roots = ?plan.created_roots, + "prepared copy archive" + ); + + let guest_archive = format!("/tmp/.agentenv-copy-{}.tar", uuid::Uuid::new_v4()); + sandbox + .upload_file(rewritten.path(), &guest_archive, "root") + .await + .with_context(|| with_step("build step failed: upload build context".to_string()))?; + + let mut script = format!( + "tar -xpf {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", + archive = shell_quote(&guest_archive), + ); + if let Some(user) = user { + let roots = plan + .created_roots + .iter() + .map(|root| shell_quote(root)) + .collect::>() + .join(" "); + if !roots.is_empty() { + script.push_str(&format!("chown -R {} {roots}\n", shell_quote(user))); + } + } + + let output = sandbox + .run_command_with_opts("/bin/bash", &["-lc", &script], &ProcessOpts::default()) + .await + .with_context(|| with_step("build step failed".to_string()))?; + if output.exit_code != 0 { + let message = format!( + "build step failed: extracting the build context exited with status {}{}", + output.exit_code, + command_output_suffix(&output.stdout, &output.stderr) + ); + return Err(with_step(message).into()); + } + Ok(()) + } + async fn run_step( &self, sandbox: &impl SandboxExecutor, @@ -103,6 +232,8 @@ impl TemplateStepExecutor { #[cfg(test)] mod tests { + use std::collections::HashMap; + use anyhow::{anyhow, Result}; use async_trait::async_trait; @@ -138,7 +269,12 @@ mod tests { async fn run(steps: Vec) -> CommandContext { TemplateStepExecutor::new() - .execute(&NoopSandbox, &steps, CommandContext::default()) + .execute( + &NoopSandbox, + &steps, + CommandContext::default(), + &HashMap::new(), + ) .await .expect("steps should execute without error") } @@ -153,7 +289,12 @@ mod tests { async fn user_step_overrides_base_image_user() { let initial = CommandContext::default().with_user(Some("root".to_string())); let ctx = TemplateStepExecutor::new() - .execute(&NoopSandbox, &[TemplateBuildStep::user("zzz")], initial) + .execute( + &NoopSandbox, + &[TemplateBuildStep::user("zzz")], + initial, + &HashMap::new(), + ) .await .unwrap(); assert_eq!(ctx.user.as_deref(), Some("zzz")); @@ -178,12 +319,46 @@ mod tests { &NoopSandbox, &[TemplateBuildStep::exposed_port("8080")], initial, + &HashMap::new(), ) .await .unwrap(); assert_eq!(ctx.exposed_ports, vec!["8080"]); } + #[tokio::test] + async fn copy_step_fails_without_uploaded_archive() { + let err = TemplateStepExecutor::new() + .execute( + &NoopSandbox, + &[TemplateBuildStep::copy( + "hello.txt", + "/hello.txt", + "aabbccddeeff0011", + None, + None, + )], + CommandContext::default(), + &HashMap::new(), + ) + .await + .expect_err("missing archive should fail the step"); + assert!(err.to_string().contains("has not been uploaded")); + } + + #[test] + fn chown_spec_validation() { + assert!(super::is_valid_chown_spec("user")); + assert!(super::is_valid_chown_spec("user:group")); + assert!(super::is_valid_chown_spec("1000:1000")); + assert!(super::is_valid_chown_spec("www-data")); + assert!(!super::is_valid_chown_spec("")); + assert!(!super::is_valid_chown_spec("user:")); + assert!(!super::is_valid_chown_spec("user:group:extra")); + assert!(!super::is_valid_chown_spec("user name")); + assert!(!super::is_valid_chown_spec("user;rm -rf /")); + } + #[tokio::test] async fn volume_deduplicates() { let ctx = run(vec![ From 205273606a2d01ecf3db067e158211cbd038e00a Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Mon, 27 Jul 2026 21:31:48 -0700 Subject: [PATCH 3/7] docs: unwrap hard-wrapped lines in the E2B template build section Match the long-line style used everywhere else in the docs tree. --- docs/src/integration/e2b.md | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/docs/src/integration/e2b.md b/docs/src/integration/e2b.md index 99738d2c..eb6da3e4 100644 --- a/docs/src/integration/e2b.md +++ b/docs/src/integration/e2b.md @@ -99,8 +99,7 @@ sandbox.kill() ### Template builds -The SDK's template builder works against AgentENV, including Dockerfiles with -`COPY`: +The SDK's template builder works against AgentENV, including Dockerfiles with `COPY`: ```python import asyncio @@ -116,28 +115,14 @@ template = Template(file_context_path=".").from_dockerfile( asyncio.run(AsyncTemplate.build(template=template, alias="my-template")) ``` -How `COPY` works: for each `COPY` instruction the SDK requests an upload link -(`GET /templates/{templateID}/files/{hash}`), `PUT`s a tar archive of the -matching context files to the returned bearer upload URL, and references the -archive by `filesHash` when it starts the build. AgentENV stores the archives -in the snapshot repository (shared across nodes) and extracts them inside the -build sandbox. +How `COPY` works: for each `COPY` instruction the SDK requests an upload link (`GET /templates/{templateID}/files/{hash}`), `PUT`s a tar archive of the matching context files to the returned bearer upload URL, and references the archive by `filesHash` when it starts the build. AgentENV stores the archives in the snapshot repository (shared across nodes) and extracts them inside the build sandbox. Requirements and behavior notes: -- The base image must provide `/bin/bash` (already required for `RUN` steps) - and `tar` for `COPY` steps. -- Copied files are owned by `root:root` like Docker's `COPY` default; - `COPY --chown=user:group` is applied with `chown -R` after extraction, so - the user must exist in the image. -- Write directory destinations with a trailing slash (`COPY app.py /opt/`). - Docker's special case of copying a single file onto an existing directory - named without a trailing slash (`COPY app.py /opt`) is not supported and - fails the build with a clear error. -- Rebuilding an existing alias is allowed: the alias keeps pointing at the - previous template while the new build runs and moves to the new template - when the build commits (E2B semantics). The previous template stays - addressable by ID. A failed rebuild leaves the alias untouched. +- The base image must provide `/bin/bash` (already required for `RUN` steps) and `tar` for `COPY` steps. +- Copied files are owned by `root:root` like Docker's `COPY` default; `COPY --chown=user:group` is applied with `chown -R` after extraction, so the user must exist in the image. +- Write directory destinations with a trailing slash (`COPY app.py /opt/`). Docker's special case of copying a single file onto an existing directory named without a trailing slash (`COPY app.py /opt`) is not supported and fails the build with a clear error. +- Rebuilding an existing alias is allowed: the alias keeps pointing at the previous template while the new build runs and moves to the new template when the build commits (E2B semantics). The previous template stays addressable by ID. A failed rebuild leaves the alias untouched. ## E2B CLI From a9227cb555410d5013e9258e74223592bf5513b7 Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Mon, 27 Jul 2026 22:03:41 -0700 Subject: [PATCH 4/7] fix(template): harden COPY step semantics and memory use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the build-context work, all in the COPY path: - `--chown` was applied as `chown -R` over the destination after extraction, so it also rewrote every pre-existing file underneath it (`COPY --chown=u app/ /usr/local/` re-owned all of /usr/local, and a destination of / re-owned the rootfs). Ownership is now resolved to numeric ids inside the build sandbox — the way Docker resolves names, against the image's own /etc/passwd and /etc/group — and written into the rewritten tar headers, so a copy can only touch what it creates. An unknown user now fails the build instead of silently landing as root, and the uploader's account names are cleared from the headers so they cannot bind to an unrelated guest account at extraction time. - The archive rewrite read every entry fully into memory and kept them all live, so a compressed upload could expand without bound on the node running the build. The rewrite now runs in two passes — index paths, then stream each entry's bytes — and rejects archives past a decompressed budget (`template_build.files_max_context_mib`, 4 GiB by default) or an unreasonable entry count. - A relative `WORKDIR` was stored verbatim, which left it undefined for RUN steps and made any later relative COPY destination fail. It now resolves against the current working directory like Docker. - A GNU sparse entry is read back expanded, so the rewritten entry is now marked as a plain regular file rather than keeping a sparse type it no longer matches. --- config/default.toml | 2 + docs/src/integration/e2b.md | 2 +- src/cfg.rs | 5 + src/template/copy_plan.rs | 449 +++++++++++++++++++++------------- src/template/step_executor.rs | 130 ++++++++-- 5 files changed, 405 insertions(+), 183 deletions(-) diff --git a/config/default.toml b/config/default.toml index e41da6b8..a058f347 100644 --- a/config/default.toml +++ b/config/default.toml @@ -84,6 +84,8 @@ peer_discovery_refresh_interval_secs = 5 # (GET /templates/{templateID}/files/{hash} plus the returned upload URL). # Maximum accepted size for one uploaded build-context archive, in MiB. # files_max_upload_mib = 1024 +# Maximum size one build-context archive may expand to once decompressed, in MiB. +# files_max_context_mib = 4096 # How long an issued upload URL stays valid, in seconds. # files_url_ttl_secs = 3600 # Optional external base URL used when building upload URLs. Defaults to diff --git a/docs/src/integration/e2b.md b/docs/src/integration/e2b.md index eb6da3e4..ba1a7a23 100644 --- a/docs/src/integration/e2b.md +++ b/docs/src/integration/e2b.md @@ -120,7 +120,7 @@ How `COPY` works: for each `COPY` instruction the SDK requests an upload link (` Requirements and behavior notes: - The base image must provide `/bin/bash` (already required for `RUN` steps) and `tar` for `COPY` steps. -- Copied files are owned by `root:root` like Docker's `COPY` default; `COPY --chown=user:group` is applied with `chown -R` after extraction, so the user must exist in the image. +- Copied files are owned by `root:root` like Docker's `COPY` default. `COPY --chown=user:group` resolves names against the image's own `/etc/passwd` and `/etc/group` and applies only to the files the copy creates; an unknown user fails the build. - Write directory destinations with a trailing slash (`COPY app.py /opt/`). Docker's special case of copying a single file onto an existing directory named without a trailing slash (`COPY app.py /opt`) is not supported and fails the build with a clear error. - Rebuilding an existing alias is allowed: the alias keeps pointing at the previous template while the new build runs and moves to the new template when the build commits (E2B semantics). The previous template stays addressable by ID. A failed rebuild leaves the alias untouched. diff --git a/src/cfg.rs b/src/cfg.rs index c1c9931a..d30f05d3 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -485,6 +485,11 @@ pub struct TemplateBuildConfig { /// Maximum accepted size for one uploaded build-context archive, in MiB. #[config(default = 1024u64)] pub files_max_upload_mib: u64, + /// Maximum size one build-context archive may expand to once + /// decompressed, in MiB. This bounds what a compressed upload can cost + /// the node that runs the build. + #[config(default = 4096u64)] + pub files_max_context_mib: u64, /// How long an issued upload URL stays valid, in seconds. #[config(default = 3600u64)] pub files_url_ttl_secs: u64, diff --git a/src/template/copy_plan.rs b/src/template/copy_plan.rs index 5d4b3436..1cb00c82 100644 --- a/src/template/copy_plan.rs +++ b/src/template/copy_plan.rs @@ -6,9 +6,12 @@ //! carries its final absolute guest path per Docker `COPY` semantics; the //! build sandbox then only needs a single `tar -xpf archive -C /`. //! -//! Ownership is normalized to root:root (Docker's `COPY` default — the SDK -//! archive carries the uploader's local uids); an explicit `--chown` user is -//! applied afterwards with `chown -R` on the created roots. +//! The rewrite runs in two passes so an archive is never held in memory: the +//! first pass indexes entry paths to compute the mapping, the second streams +//! each entry's bytes straight into the rewritten archive. +//! +//! Ownership is written into the rewritten headers rather than applied with a +//! post-extract `chown`, so a copy can only ever change the files it creates. use std::fs::File; use std::io::{BufReader, Read, Seek, SeekFrom, Write}; @@ -16,25 +19,48 @@ use std::path::Path; use anyhow::{bail, Context, Result}; +/// Upper bound on entries in one build-context archive. The indexing pass +/// keeps one normalized path per entry, so this bounds that allocation +/// independently of the byte budget: an archive of a million empty files is +/// tiny but path-heavy. Real build contexts are orders of magnitude smaller. +const MAX_ARCHIVE_ENTRIES: usize = 200_000; + +/// Numeric ownership applied to every entry of one copy. +#[derive(Clone, Copy, Debug)] +pub(crate) struct CopyOwnership { + pub(crate) uid: u64, + pub(crate) gid: u64, +} + +/// Inputs for rewriting one `COPY` step's archive. +pub(crate) struct CopyRequest<'a> { + pub(crate) source_tar: &'a Path, + pub(crate) src: &'a str, + pub(crate) dest: &'a str, + pub(crate) workdir: &'a str, + pub(crate) mode: Option, + /// Ownership requested by `--chown`, already resolved to numeric ids + /// inside the build sandbox. `None` keeps Docker's root:root default. + pub(crate) ownership: Option, + /// Budget for the decompressed archive, bounding both the rewritten file + /// on the host and what a single upload can expand to. + pub(crate) max_total_bytes: u64, +} + /// Summary of a rewritten copy archive. #[derive(Debug)] pub(crate) struct CopyPlan { - /// Absolute guest paths of the top-level items this copy creates, used - /// for the optional post-extract `chown -R`. - pub(crate) created_roots: Vec, /// Number of file/dir/symlink entries written to the rewritten archive. pub(crate) entry_count: usize, + /// Total file bytes written to the rewritten archive. + pub(crate) total_bytes: u64, } -/// One entry read from the SDK context archive. -struct SourceEntry { +/// One archive entry as seen by the indexing pass. +struct EntryIndex { /// Normalized context-relative path ("dir/file.txt"). path: String, - header: tar::Header, - link_name: Option, - /// Byte range of the entry data within the decompressed stream is not - /// seekable, so file contents are buffered per entry during rewrite. - data: Vec, + is_dir: bool, } fn is_glob_pattern(src: &str) -> bool { @@ -101,20 +127,18 @@ fn normalize_src(src: &str) -> String { src.trim_end_matches('/').to_string() } -/// Joins and lexically normalizes an absolute guest destination path. -fn resolve_dest(dest: &str, workdir: &str) -> Result { - let joined = if dest.starts_with('/') { - dest.to_string() +/// Joins `path` onto `base` and lexically normalizes the result into an +/// absolute guest path. Absolute `path` values replace `base` entirely, which +/// is how Docker resolves both `WORKDIR` and `COPY` destinations. +pub(crate) fn resolve_guest_path(base: &str, path: &str) -> Result { + let joined = if path.starts_with('/') { + path.to_string() } else { - let workdir = if workdir.trim().is_empty() { - "/" - } else { - workdir - }; - if !workdir.starts_with('/') { - bail!("workdir '{workdir}' must be absolute to resolve relative COPY destination"); + let base = if base.trim().is_empty() { "/" } else { base }; + if !base.starts_with('/') { + bail!("cannot resolve '{path}' against non-absolute base '{base}'"); } - format!("{}/{}", workdir.trim_end_matches('/'), dest) + format!("{}/{}", base.trim_end_matches('/'), path) }; let mut parts: Vec<&str> = Vec::new(); @@ -123,7 +147,7 @@ fn resolve_dest(dest: &str, workdir: &str) -> Result { "" | "." => {} ".." => { if parts.pop().is_none() { - bail!("COPY destination '{dest}' escapes the filesystem root"); + bail!("path '{path}' escapes the filesystem root"); } } part => parts.push(part), @@ -168,7 +192,8 @@ fn normalize_entry_path(raw: &Path) -> Result { Ok(parts.join("/")) } -fn read_source_entries(source_tar: &Path) -> Result> { +/// Opens the uploaded archive, transparently decompressing gzip. +fn open_archive(source_tar: &Path) -> Result>> { let mut file = File::open(source_tar) .with_context(|| format!("open build context archive '{}'", source_tar.display()))?; let mut magic = [0u8; 2]; @@ -184,93 +209,96 @@ fn read_source_entries(source_tar: &Path) -> Result> { } else { Box::new(BufReader::new(file)) }; + Ok(tar::Archive::new(reader)) +} + +fn check_entry_type(entry_type: tar::EntryType) -> Result<()> { + match entry_type { + tar::EntryType::Regular + | tar::EntryType::Directory + | tar::EntryType::Symlink + | tar::EntryType::GNUSparse => Ok(()), + // Metadata-only companion entries (long names, pax headers) are + // consumed by the tar crate itself and never surface here. + other => bail!("unsupported entry type {other:?} in build context archive"), + } +} + +/// First pass: index entry paths and enforce the archive budgets without +/// reading any file contents. +fn read_entry_index(source_tar: &Path, max_total_bytes: u64) -> Result> { + let mut archive = open_archive(source_tar)?; + let mut index = Vec::new(); + let mut total_bytes = 0u64; - let mut archive = tar::Archive::new(reader); - let mut entries = Vec::new(); for entry in archive .entries() .context("read build context archive entries")? { - let mut entry = entry.context("read build context archive entry")?; + let entry = entry.context("read build context archive entry")?; let entry_type = entry.header().entry_type(); - match entry_type { - tar::EntryType::Regular - | tar::EntryType::Directory - | tar::EntryType::Symlink - | tar::EntryType::GNUSparse => {} - // Metadata-only companion entries (long names, pax headers) are - // consumed by the tar crate itself and never surface here. - other => bail!("unsupported entry type {other:?} in build context archive"), - } + check_entry_type(entry_type)?; - let path = normalize_entry_path(&entry.path().context("entry path")?)?; - let link_name = entry - .link_name() - .context("entry link name")? - .map(|l| l.into_owned()); - let mut data = Vec::new(); - if entry_type == tar::EntryType::Regular || entry_type == tar::EntryType::GNUSparse { - entry.read_to_end(&mut data).context("entry contents")?; + total_bytes = total_bytes.saturating_add(entry.size()); + if total_bytes > max_total_bytes { + bail!( + "build context archive expands beyond the configured limit of \ + {max_total_bytes} bytes" + ); + } + if index.len() >= MAX_ARCHIVE_ENTRIES { + bail!("build context archive holds more than {MAX_ARCHIVE_ENTRIES} entries"); } - entries.push(SourceEntry { - path, - header: entry.header().clone(), - link_name, - data, + + index.push(EntryIndex { + path: normalize_entry_path(&entry.path().context("entry path")?)?, + is_dir: entry_type == tar::EntryType::Directory, }); } - if entries.is_empty() { + + if index.is_empty() { bail!("build context archive contains no files"); } - Ok(entries) + Ok(index) } -/// Computes the final absolute guest path for every entry. +/// Computes the final absolute guest path for every indexed entry. /// -/// Returns `(mappings, created_roots)` where `mappings[i]` matches -/// `entries[i]`. +/// The returned vector is positionally aligned with `index`. fn map_entries( - entries: &[SourceEntry], + index: &[EntryIndex], src: &str, dest_raw: &str, workdir: &str, -) -> Result<(Vec, Vec)> { +) -> Result> { let src = normalize_src(src); let dest_is_dir_hint = dest_raw.ends_with('/') || dest_raw.ends_with("/.") || dest_raw == "." || dest_raw.is_empty(); - let dest = resolve_dest(if dest_raw.is_empty() { "." } else { dest_raw }, workdir)?; + let dest = resolve_guest_path(workdir, if dest_raw.is_empty() { "." } else { dest_raw })?; - let mut mapped = Vec::with_capacity(entries.len()); - let mut roots: Vec = Vec::new(); - let mut push_root = |root: String| { - if !roots.contains(&root) { - roots.push(root); - } - }; + let mut mapped = Vec::with_capacity(index.len()); let copy_whole_context = src.is_empty() || src == "."; let single_file_src = !copy_whole_context && !is_glob_pattern(&src) - && entries.len() == 1 - && entries[0].path == src - && entries[0].header.entry_type() != tar::EntryType::Directory; + && index.len() == 1 + && index[0].path == src + && !index[0].is_dir; if single_file_src { - let target = if dest_is_dir_hint { + mapped.push(if dest_is_dir_hint { join_abs(&dest, base_name(&src)) } else { - dest.clone() - }; - push_root(target.clone()); - mapped.push(target); - return Ok((mapped, roots)); + dest + }); + return Ok(mapped); } if copy_whole_context || !is_glob_pattern(&src) { // Directory source: Docker copies the directory *contents* into dest. - for entry in entries { + for entry in index { let rel = if copy_whole_context { entry.path.as_str() } else if entry.path == src { @@ -286,14 +314,13 @@ fn map_entries( }; mapped.push(join_abs(&dest, rel)); } - push_root(dest.clone()); - return Ok((mapped, roots)); + return Ok(mapped); } // Glob source: every matched top-level item lands inside dest. Matched // files keep their base name; matched directories contribute their // contents (Docker treats each matched directory like a directory source). - for entry in entries { + for entry in index { let mut components = entry.path.split('/'); let mut prefix = String::new(); let mut matched_root: Option = None; @@ -321,54 +348,71 @@ fn map_entries( .strip_prefix(&root) .map(|rest| rest.trim_start_matches('/')) .unwrap_or(""); - let target = if rel.is_empty() && entry.header.entry_type() != tar::EntryType::Directory { - let target = join_abs(&dest, base_name(&root)); - push_root(target.clone()); - target + mapped.push(if rel.is_empty() && !entry.is_dir { + join_abs(&dest, base_name(&root)) } else { - push_root(dest.clone()); join_abs(&dest, rel) - }; - mapped.push(target); + }); } - Ok((mapped, roots)) + Ok(mapped) } /// Rewrites the SDK context archive into `output` with final absolute guest -/// paths, root ownership, and the optional mode override applied. -pub(crate) fn plan_copy_archive( - source_tar: &Path, - src: &str, - dest: &str, - workdir: &str, - mode_override: Option, - output: &Path, -) -> Result { - let entries = read_source_entries(source_tar)?; - let (mapped, created_roots) = map_entries(&entries, src, dest, workdir)?; +/// paths, the requested ownership, and the optional mode override applied. +pub(crate) fn plan_copy_archive(request: &CopyRequest<'_>, output: &Path) -> Result { + let index = read_entry_index(request.source_tar, request.max_total_bytes)?; + let targets = map_entries(&index, request.src, request.dest, request.workdir)?; let out_file = File::create(output) .with_context(|| format!("create rewritten copy archive '{}'", output.display()))?; let mut builder = tar::Builder::new(out_file); let mut entry_count = 0usize; + let mut total_bytes = 0u64; + let mut seen = 0usize; + + // Second pass: stream each entry's bytes into the rewritten archive. + let mut archive = open_archive(request.source_tar)?; + for entry in archive + .entries() + .context("read build context archive entries")? + { + let mut entry = entry.context("read build context archive entry")?; + let Some(target) = targets.get(seen) else { + bail!("build context archive changed while it was being rewritten"); + }; + seen += 1; - for (entry, target) in entries.iter().zip(mapped.iter()) { let relative = target.trim_start_matches('/'); if relative.is_empty() { // The destination root itself ("/"); parents always exist. continue; } - let mut header = entry.header.clone(); - header.set_uid(0); - header.set_gid(0); - let _ = header.set_username(""); - let _ = header.set_groupname(""); - if let Some(mode) = mode_override { + let entry_type = entry.header().entry_type(); + check_entry_type(entry_type)?; + let link_name = entry + .link_name() + .context("entry link name")? + .map(|link| link.into_owned()); + let mut header = entry.header().clone(); + let (uid, gid) = request + .ownership + .map_or((0, 0), |owner| (owner.uid, owner.gid)); + header.set_uid(uid); + header.set_gid(gid); + // Clear the name fields so the numeric ids above are authoritative. + // GNU tar prefers uname/gname when they resolve in the target image, + // so leaving the uploader's account names in place could hand files + // to an unrelated guest account. + header + .set_username("") + .and_then(|()| header.set_groupname("")) + .with_context(|| format!("clear ownership names on entry '{target}'"))?; + if let Some(mode) = request.mode { header.set_mode(mode); } - match entry.header.entry_type() { + match entry_type { tar::EntryType::Directory => { header.set_size(0); builder @@ -376,31 +420,37 @@ pub(crate) fn plan_copy_archive( .with_context(|| format!("write directory entry '{target}'"))?; } tar::EntryType::Symlink => { - let link = entry - .link_name - .as_ref() - .context("symlink entry is missing its target")?; + let link = link_name.context("symlink entry is missing its target")?; header.set_size(0); builder - .append_link(&mut header, relative, link) + .append_link(&mut header, relative, &link) .with_context(|| format!("write symlink entry '{target}'"))?; } _ => { - header.set_size(entry.data.len() as u64); + let size = entry.size(); + header.set_size(size); + // A GNU sparse entry is read back expanded, so the rewritten + // entry is a plain regular file. + header.set_entry_type(tar::EntryType::Regular); builder - .append_data(&mut header, relative, entry.data.as_slice()) + .append_data(&mut header, relative, &mut entry) .with_context(|| format!("write file entry '{target}'"))?; + total_bytes += size; } } entry_count += 1; } + if seen != targets.len() { + bail!("build context archive changed while it was being rewritten"); + } + let mut out_file = builder.into_inner().context("finish rewritten archive")?; out_file.flush().context("flush rewritten archive")?; Ok(CopyPlan { - created_roots, entry_count, + total_bytes, }) } @@ -410,6 +460,25 @@ mod tests { use std::collections::BTreeMap; use tempfile::TempDir; + const NO_LIMIT: u64 = u64::MAX; + + fn request<'a>( + source_tar: &'a Path, + src: &'a str, + dest: &'a str, + workdir: &'a str, + ) -> CopyRequest<'a> { + CopyRequest { + source_tar, + src, + dest, + workdir, + mode: None, + ownership: None, + max_total_bytes: NO_LIMIT, + } + } + fn build_source_tar(dir: &Path, entries: &[(&str, Option<&str>)]) -> std::path::PathBuf { // (path, Some(contents)) = file, (path, None) = directory let tar_path = dir.join("source.tar"); @@ -442,7 +511,15 @@ mod tests { tar_path } - fn rewritten_entries(path: &Path) -> BTreeMap { + struct Rewritten { + kind: tar::EntryType, + uid: u64, + gid: u64, + mode: u32, + contents: String, + } + + fn rewritten_entries(path: &Path) -> BTreeMap { let mut archive = tar::Archive::new(File::open(path).expect("open rewritten")); let mut out = BTreeMap::new(); for entry in archive.entries().expect("entries") { @@ -450,10 +527,20 @@ mod tests { let path = entry.path().expect("path").to_string_lossy().into_owned(); let kind = entry.header().entry_type(); let uid = entry.header().uid().expect("uid"); + let gid = entry.header().gid().expect("gid"); let mode = entry.header().mode().expect("mode"); let mut contents = String::new(); entry.read_to_string(&mut contents).expect("read"); - out.insert(path, (kind, uid, mode, contents)); + out.insert( + path, + Rewritten { + kind, + uid, + gid, + mode, + contents, + }, + ); } out } @@ -465,15 +552,16 @@ mod tests { let out = dir.path().join("out.tar"); let plan = - plan_copy_archive(&tar, "hello.txt", "/hello.txt", "/", None, &out).expect("plan"); + plan_copy_archive(&request(&tar, "hello.txt", "/hello.txt", "/"), &out).expect("plan"); assert_eq!(plan.entry_count, 1); - assert_eq!(plan.created_roots, vec!["/hello.txt".to_string()]); + assert_eq!(plan.total_bytes, 6); let entries = rewritten_entries(&out); - let (kind, uid, _, contents) = &entries["hello.txt"]; - assert_eq!(*kind, tar::EntryType::Regular); - assert_eq!(*uid, 0, "ownership must be normalized to root"); - assert_eq!(contents, "hello\n"); + let entry = &entries["hello.txt"]; + assert_eq!(entry.kind, tar::EntryType::Regular); + assert_eq!(entry.uid, 0, "ownership must default to root"); + assert_eq!(entry.gid, 0); + assert_eq!(entry.contents, "hello\n"); } #[test] @@ -482,15 +570,10 @@ mod tests { let tar = build_source_tar(dir.path(), &[("requirements.txt", Some("e2b\n"))]); let out = dir.path().join("out.tar"); - let plan = plan_copy_archive(&tar, "requirements.txt", "/home/user/", "/", None, &out) + plan_copy_archive(&request(&tar, "requirements.txt", "/home/user/", "/"), &out) .expect("plan"); - assert_eq!( - plan.created_roots, - vec!["/home/user/requirements.txt".to_string()] - ); - let entries = rewritten_entries(&out); - assert!(entries.contains_key("home/user/requirements.txt")); + assert!(rewritten_entries(&out).contains_key("home/user/requirements.txt")); } #[test] @@ -499,12 +582,9 @@ mod tests { let tar = build_source_tar(dir.path(), &[("config.py", Some("x = 1\n"))]); let out = dir.path().join("out.tar"); - let plan = - plan_copy_archive(&tar, "config.py", "conf/app.py", "/srv", None, &out).expect("plan"); + plan_copy_archive(&request(&tar, "config.py", "conf/app.py", "/srv"), &out).expect("plan"); - assert_eq!(plan.created_roots, vec!["/srv/conf/app.py".to_string()]); - let entries = rewritten_entries(&out); - assert!(entries.contains_key("srv/conf/app.py")); + assert!(rewritten_entries(&out).contains_key("srv/conf/app.py")); } #[test] @@ -521,9 +601,8 @@ mod tests { ); let out = dir.path().join("out.tar"); - let plan = plan_copy_archive(&tar, "app", "/opt/service", "/", None, &out).expect("plan"); + plan_copy_archive(&request(&tar, "app", "/opt/service", "/"), &out).expect("plan"); - assert_eq!(plan.created_roots, vec!["/opt/service".to_string()]); let entries = rewritten_entries(&out); assert!(entries.contains_key("opt/service/")); assert!(entries.contains_key("opt/service/main.py")); @@ -544,9 +623,8 @@ mod tests { ); let out = dir.path().join("out.tar"); - let plan = plan_copy_archive(&tar, ".", "/workspace", "/", None, &out).expect("plan"); + plan_copy_archive(&request(&tar, ".", "/workspace", "/"), &out).expect("plan"); - assert_eq!(plan.created_roots, vec!["/workspace".to_string()]); let entries = rewritten_entries(&out); assert!(entries.contains_key("workspace/a.txt")); assert!(entries.contains_key("workspace/sub/b.txt")); @@ -561,16 +639,12 @@ mod tests { ); let out = dir.path().join("out.tar"); - let plan = plan_copy_archive(&tar, "*.txt", "/data/", "/", None, &out).expect("plan"); + let plan = plan_copy_archive(&request(&tar, "*.txt", "/data/", "/"), &out).expect("plan"); assert_eq!(plan.entry_count, 2); let entries = rewritten_entries(&out); assert!(entries.contains_key("data/one.txt")); assert!(entries.contains_key("data/two.txt")); - assert_eq!( - plan.created_roots, - vec!["/data/one.txt".to_string(), "/data/two.txt".to_string()] - ); } #[test] @@ -587,13 +661,11 @@ mod tests { ); let out = dir.path().join("out.tar"); - let plan = plan_copy_archive(&tar, "pkg-*", "/opt/pkgs", "/", None, &out).expect("plan"); + plan_copy_archive(&request(&tar, "pkg-*", "/opt/pkgs", "/"), &out).expect("plan"); - let entries = rewritten_entries(&out); // Docker merges contents of every matched directory into dest; the // second lib.py overwrites the first at extract time. - assert!(entries.contains_key("opt/pkgs/lib.py")); - assert_eq!(plan.created_roots, vec!["/opt/pkgs".to_string()]); + assert!(rewritten_entries(&out).contains_key("opt/pkgs/lib.py")); } #[test] @@ -602,19 +674,35 @@ mod tests { let tar = build_source_tar(dir.path(), &[("run.sh", Some("#!/bin/sh\n"))]); let out = dir.path().join("out.tar"); - plan_copy_archive( - &tar, - "run.sh", - "/usr/local/bin/run.sh", - "/", - Some(0o755), - &out, - ) - .expect("plan"); + let mut req = request(&tar, "run.sh", "/usr/local/bin/run.sh", "/"); + req.mode = Some(0o755); + plan_copy_archive(&req, &out).expect("plan"); - let entries = rewritten_entries(&out); - let (_, _, mode, _) = &entries["usr/local/bin/run.sh"]; - assert_eq!(*mode, 0o755); + assert_eq!(rewritten_entries(&out)["usr/local/bin/run.sh"].mode, 0o755); + } + + #[test] + fn ownership_is_written_into_entry_headers() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar( + dir.path(), + &[("app", None), ("app/main.py", Some("print()\n"))], + ); + let out = dir.path().join("out.tar"); + + let mut req = request(&tar, "app", "/opt/service", "/"); + req.ownership = Some(CopyOwnership { + uid: 1000, + gid: 2000, + }); + plan_copy_archive(&req, &out).expect("plan"); + + // Every created entry carries the requested ownership, and nothing + // outside the archive can be affected. + for entry in rewritten_entries(&out).values() { + assert_eq!(entry.uid, 1000); + assert_eq!(entry.gid, 2000); + } } #[test] @@ -630,11 +718,23 @@ mod tests { encoder.finish().expect("finish gz"); let out = dir.path().join("out.tar"); - let plan = - plan_copy_archive(&gz_path, "hello.txt", "/hello.txt", "/", None, &out).expect("plan"); + let plan = plan_copy_archive(&request(&gz_path, "hello.txt", "/hello.txt", "/"), &out) + .expect("plan"); assert_eq!(plan.entry_count, 1); } + #[test] + fn rejects_archives_over_the_decompressed_budget() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("big.txt", Some("0123456789"))]); + let out = dir.path().join("out.tar"); + + let mut req = request(&tar, "big.txt", "/big.txt", "/"); + req.max_total_bytes = 4; + let err = plan_copy_archive(&req, &out).expect_err("oversized archive must fail"); + assert!(err.to_string().contains("expands beyond")); + } + #[test] fn rejects_entries_escaping_the_root() { let dir = TempDir::new().expect("tempdir"); @@ -656,7 +756,7 @@ mod tests { builder.finish().expect("finish"); let out = dir.path().join("out.tar"); - let err = plan_copy_archive(&tar_path, "passwd", "/tmp/x", "/", None, &out) + let err = plan_copy_archive(&request(&tar_path, "passwd", "/tmp/x", "/"), &out) .expect_err("path escape must fail"); assert!(err.to_string().contains("unsupported path component")); } @@ -667,7 +767,7 @@ mod tests { let tar = build_source_tar(dir.path(), &[("a.txt", Some("a"))]); let out = dir.path().join("out.tar"); - let err = plan_copy_archive(&tar, "a.txt", "../../x", "/", None, &out) + let err = plan_copy_archive(&request(&tar, "a.txt", "../../x", "/"), &out) .expect_err("dest escape must fail"); assert!(err.to_string().contains("escapes the filesystem root")); } @@ -680,11 +780,30 @@ mod tests { tar::Builder::new(file).finish().expect("finish"); let out = dir.path().join("out.tar"); - let err = plan_copy_archive(&tar_path, "x", "/x", "/", None, &out) + let err = plan_copy_archive(&request(&tar_path, "x", "/x", "/"), &out) .expect_err("empty archive must fail"); assert!(err.to_string().contains("no files")); } + #[test] + fn guest_path_resolution_follows_docker_semantics() { + assert_eq!( + resolve_guest_path("/srv", "app").expect("relative"), + "/srv/app" + ); + assert_eq!( + resolve_guest_path("/srv/app", "/opt").expect("absolute"), + "/opt" + ); + assert_eq!( + resolve_guest_path("/srv/app", "../lib").expect("parent"), + "/srv/lib" + ); + assert_eq!(resolve_guest_path("", "opt").expect("empty base"), "/opt"); + assert!(resolve_guest_path("relative", "app").is_err()); + assert!(resolve_guest_path("/", "../escape").is_err()); + } + #[test] fn glob_match_basics() { assert!(glob_match("*.txt", "a.txt")); diff --git a/src/template/step_executor.rs b/src/template/step_executor.rs index b74e93f6..f3b2adc3 100644 --- a/src/template/step_executor.rs +++ b/src/template/step_executor.rs @@ -6,8 +6,9 @@ use shell_util::shell_quote; use tracing::debug; use super::build_spec::{TemplateBuildStep, TemplateBuildStepKind}; -use super::copy_plan::plan_copy_archive; +use super::copy_plan::{plan_copy_archive, resolve_guest_path, CopyOwnership, CopyRequest}; use super::errors::{command_output_suffix, TemplateBuildFailure}; +use crate::cfg::ConfigManager; use crate::sandbox::{ProcessOpts, SandboxExecutor}; use crate::snapshot::CommandContext; @@ -62,7 +63,17 @@ impl TemplateStepExecutor { context = context.with_env_var(key.clone(), value.clone()); } TemplateBuildStepKind::Workdir { path } => { - context = context.with_workdir(path.to_string_lossy()); + // Docker resolves a relative WORKDIR against the current + // one; only absolute values replace it outright. + let path = path.to_string_lossy(); + let resolved = + resolve_guest_path(&context.workdir, &path).with_context(|| { + TemplateBuildFailure::with_step( + format!("build step failed: invalid workdir '{path}'"), + format!("WORKDIR {path}"), + ) + })?; + context = context.with_workdir(resolved); } TemplateBuildStepKind::User { value } => { context = context.with_user(Some(value.clone())); @@ -148,17 +159,40 @@ impl TemplateStepExecutor { } } + // Resolve --chown against the image's own accounts, like Docker, and + // bake the numeric result into the archive headers. Applying it after + // extraction with `chown -R` would also rewrite pre-existing files + // under the destination. + let ownership = match user { + Some(user) => Some(self.resolve_ownership(sandbox, user, &step_label).await?), + None => None, + }; + let rewritten = tempfile::Builder::new() .prefix("agentenv-copy-") .suffix(".tar") .tempfile() .context("create rewritten copy archive")?; - let plan = plan_copy_archive(archive, src, dest, &context.workdir, mode, rewritten.path()) - .map_err(|error| with_step(format!("build step failed: {error:#}")))?; + let plan = plan_copy_archive( + &CopyRequest { + source_tar: archive, + src, + dest, + workdir: &context.workdir, + mode, + ownership, + max_total_bytes: ConfigManager::global_config() + .template_build + .files_max_context_mib + .saturating_mul(1024 * 1024), + }, + rewritten.path(), + ) + .map_err(|error| with_step(format!("build step failed: {error:#}")))?; debug!( files_hash, entries = plan.entry_count, - roots = ?plan.created_roots, + bytes = plan.total_bytes, "prepared copy archive" ); @@ -168,21 +202,10 @@ impl TemplateStepExecutor { .await .with_context(|| with_step("build step failed: upload build context".to_string()))?; - let mut script = format!( + let script = format!( "tar -xpf {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", archive = shell_quote(&guest_archive), ); - if let Some(user) = user { - let roots = plan - .created_roots - .iter() - .map(|root| shell_quote(root)) - .collect::>() - .join(" "); - if !roots.is_empty() { - script.push_str(&format!("chown -R {} {roots}\n", shell_quote(user))); - } - } let output = sandbox .run_command_with_opts("/bin/bash", &["-lc", &script], &ProcessOpts::default()) @@ -199,6 +222,79 @@ impl TemplateStepExecutor { Ok(()) } + /// Resolves a `--chown` spec to numeric ids inside the build sandbox. + /// + /// Docker resolves names against the image's own `/etc/passwd` and + /// `/etc/group`, so the lookup has to happen in the guest. Failing here + /// reports an unknown user the same way Docker does rather than silently + /// falling back to root. + async fn resolve_ownership( + &self, + sandbox: &impl SandboxExecutor, + user: &str, + step_label: &str, + ) -> Result { + let (user_part, group_part) = user.split_once(':').unwrap_or((user, "")); + let script = format!( + r#"set -eu +case "{user}" in + *[!0-9]*) uid=$(id -u "{user}"); ugid=$(id -g "{user}") ;; + *) uid="{user}"; ugid="{user}" ;; +esac +if [ -n "{group}" ]; then + case "{group}" in + *[!0-9]*) gid=$(awk -F: -v n="{group}" '$1==n{{print $3; f=1}} END{{exit !f}}' /etc/group) ;; + *) gid="{group}" ;; + esac +else + gid="$ugid" +fi +printf '%s %s\n' "$uid" "$gid" +"#, + user = user_part, + group = group_part, + ); + + let output = sandbox + .run_command_with_opts("/bin/bash", &["-lc", &script], &ProcessOpts::default()) + .await + .with_context(|| { + TemplateBuildFailure::with_step( + "build step failed: resolve COPY ownership".to_string(), + step_label, + ) + })?; + if output.exit_code != 0 { + return Err(TemplateBuildFailure::with_step( + format!( + "build step failed: COPY user '{user}' does not exist in the image{}", + command_output_suffix(&output.stdout, &output.stderr) + ), + step_label, + ) + .into()); + } + + let parsed = output + .stdout + .split_whitespace() + .map(str::parse::) + .collect::, _>>() + .ok() + .filter(|ids| ids.len() == 2); + let Some(ids) = parsed else { + return Err(TemplateBuildFailure::with_step( + format!("build step failed: could not resolve COPY user '{user}'"), + step_label, + ) + .into()); + }; + Ok(CopyOwnership { + uid: ids[0], + gid: ids[1], + }) + } + async fn run_step( &self, sandbox: &impl SandboxExecutor, From fb248599fe0a8880b5d636b1b8ab06f956b67fe6 Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Mon, 27 Jul 2026 22:05:21 -0700 Subject: [PATCH 5/7] fix(template): make build-context upload URLs single-use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An upload grant was validated but never consumed, so an upload URL kept working for its whole TTL and could re-upload under the same hash; `import` overwrote unconditionally, which could also swap an archive out from under a build already reading it. Claiming a grant now consumes it — on POSIX the `remove_file` that consumes it is the atomic claim, so concurrent replays of one token cannot both win — and stored archives are immutable. Since a hash addresses its content, keeping the first archive costs nothing and means no build can observe its context change while it runs. Verifying that an upload matches its `filesHash` was considered instead and rejected: the SDK derives that hash from the COPY arguments plus per-file metadata and content in its own traversal order, not from the archive bytes, so recomputing it server-side would pin us to SDK internals. --- src/api/build_files.rs | 7 +- .../repository/backends/oss/build_files.rs | 25 +++++- .../backends/posixfs/build_files.rs | 88 ++++++++++++++++--- src/snapshot/repository/build_files.rs | 16 +++- 4 files changed, 116 insertions(+), 20 deletions(-) diff --git a/src/api/build_files.rs b/src/api/build_files.rs index cacd7ca7..9e0c3798 100644 --- a/src/api/build_files.rs +++ b/src/api/build_files.rs @@ -92,14 +92,15 @@ where ); }; + // Claiming consumes the grant, so an upload URL works exactly once. let now_unix = chrono::Utc::now().timestamp(); let authorized = match store - .validate_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) + .claim_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) .await { Ok(authorized) => authorized, Err(error) => { - warn!(error = %error, "failed to validate build-file upload grant"); + warn!(error = %error, "failed to claim build-file upload grant"); return error_response( StatusCode::INTERNAL_SERVER_ERROR, "failed to validate upload grant", @@ -109,7 +110,7 @@ where if !authorized { return error_response( StatusCode::UNAUTHORIZED, - "upload grant is invalid or expired; request a fresh upload link", + "upload grant is invalid, expired, or already used; request a fresh upload link", ); } diff --git a/src/snapshot/repository/backends/oss/build_files.rs b/src/snapshot/repository/backends/oss/build_files.rs index 9654fe8f..4e9294a5 100644 --- a/src/snapshot/repository/backends/oss/build_files.rs +++ b/src/snapshot/repository/backends/oss/build_files.rs @@ -53,6 +53,16 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()> { let key = Self::archive_key(hash)?; + // Archives are immutable: the hash addresses the content, so a repeat + // upload cannot change what an in-flight build reads. + if self + .client + .exists(&key) + .await + .map_err(|error| RepositoryError::backend("check build archive", error))? + { + return Ok(()); + } self.client .put_file(&key, staged) .await @@ -102,7 +112,7 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { Ok(token) } - async fn validate_upload_grant( + async fn claim_upload_grant( &self, token: &str, template_id: &str, @@ -120,6 +130,17 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { }; let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) .map_err(|error| RepositoryError::backend("parse upload grant", error))?; - Ok(grant.authorizes(template_id, hash, expires_unix, now_unix)) + if !grant.authorizes(template_id, hash, expires_unix, now_unix) { + return Ok(false); + } + // Consume the grant so the upload URL cannot be replayed. S3-compatible + // stores offer no conditional delete, so simultaneous replays of one + // token can both observe the grant; archives are immutable, which is + // what keeps that from mattering. + self.client + .delete(&key) + .await + .map_err(|error| RepositoryError::backend("consume upload grant", error))?; + Ok(true) } } diff --git a/src/snapshot/repository/backends/posixfs/build_files.rs b/src/snapshot/repository/backends/posixfs/build_files.rs index ae8d4cbf..a40298b7 100644 --- a/src/snapshot/repository/backends/posixfs/build_files.rs +++ b/src/snapshot/repository/backends/posixfs/build_files.rs @@ -171,6 +171,11 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { let root = self.root.clone(); let staged = staged.to_path_buf(); task::spawn_blocking(move || -> RepositoryResult<()> { + // Archives are immutable: the hash addresses the content, so a + // repeat upload cannot change what an in-flight build reads. + if final_path.exists() { + return Ok(()); + } Self::ensure_root(&root)?; Self::prune_expired(&root); // Copy into the store filesystem first (the staged file usually @@ -215,7 +220,7 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { .map_err(|error| RepositoryError::backend("join create upload grant task", error))? } - async fn validate_upload_grant( + async fn claim_upload_grant( &self, token: &str, template_id: &str, @@ -238,10 +243,20 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { }; let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) .map_err(|error| RepositoryError::backend("parse upload grant", error))?; - Ok(grant.authorizes(&template_id, &hash, expires_unix, now_unix)) + if !grant.authorizes(&template_id, &hash, expires_unix, now_unix) { + return Ok(false); + } + // Consume the grant. `remove_file` succeeds for exactly one + // caller, so it is the claim: concurrent replays of the same + // token lose the race and are rejected. + match fs::remove_file(&path) { + Ok(()) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(RepositoryError::backend("consume upload grant", error)), + } }) .await - .map_err(|error| RepositoryError::backend("join validate upload grant task", error))? + .map_err(|error| RepositoryError::backend("join claim upload grant task", error))? } } @@ -338,7 +353,7 @@ mod tests { assert!(!stale_path.exists(), "expired grant file should be pruned"); assert!( store - .validate_upload_grant(&fresh_token, "template", HASH, i64::MAX, 0) + .claim_upload_grant(&fresh_token, "template", HASH, i64::MAX, 0) .await .expect("validation should work"), "unexpired grants must survive pruning" @@ -349,22 +364,73 @@ mod tests { async fn upload_grant_is_shared_across_instances() { let tempdir = TempDir::new().expect("tempdir"); let first = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let second = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + // A mismatched or expired claim leaves the grant usable. let token = first .create_upload_grant("template", HASH, 1000) .await .expect("grant should be created"); - let second = PosixFsTemplateBuildFileStore::new(tempdir.path()); - assert!(second - .validate_upload_grant(&token, "template", HASH, 1000, 999) - .await - .expect("grant should validate")); assert!(!second - .validate_upload_grant(&token, "other", HASH, 1000, 999) + .claim_upload_grant(&token, "other", HASH, 1000, 999) .await .expect("mismatched grant should be rejected")); assert!(!second - .validate_upload_grant(&token, "template", HASH, 1000, 1001) + .claim_upload_grant(&token, "template", HASH, 1000, 1001) .await .expect("expired grant should be rejected")); + assert!(second + .claim_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("grant issued by another instance should claim")); + } + + #[tokio::test] + async fn upload_grant_is_single_use() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let token = store + .create_upload_grant("template", HASH, 1000) + .await + .expect("grant should be created"); + + assert!(store + .claim_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("first claim should succeed")); + assert!( + !store + .claim_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("replay should be rejected"), + "an upload URL must not be replayable" + ); + } + + #[tokio::test] + async fn archives_are_immutable_once_stored() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + let first = staged_file(tempdir.path(), b"original"); + store.import(HASH, &first).await.expect("first import"); + + let replacement = tempdir.path().join("replacement.tar"); + fs::write(&replacement, b"replaced").expect("write replacement"); + store + .import(HASH, &replacement) + .await + .expect("repeat import should be accepted"); + + let materialized = store + .materialize(HASH, tempdir.path()) + .await + .expect("materialize should work") + .expect("archive should exist"); + assert_eq!( + fs::read(materialized).expect("read materialized"), + b"original", + "a stored archive must never be replaced underneath a build" + ); } } diff --git a/src/snapshot/repository/build_files.rs b/src/snapshot/repository/build_files.rs index 71a47841..9395129a 100644 --- a/src/snapshot/repository/build_files.rs +++ b/src/snapshot/repository/build_files.rs @@ -66,8 +66,10 @@ pub trait TemplateBuildFileStore: Send + Sync { /// Imports a fully written local file as the archive for `hash`. /// /// Implementations must publish atomically: concurrent readers never - /// observe a partially imported archive. Re-importing an existing hash - /// replaces the stored archive. + /// observe a partially imported archive. Archives are content-addressed + /// and therefore immutable — importing a hash that is already stored + /// keeps the stored archive, so an in-flight build can never observe its + /// build context change underneath it. async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; /// Materializes the archive for `hash` as a node-local file. @@ -91,8 +93,14 @@ pub trait TemplateBuildFileStore: Send + Sync { expires_unix: i64, ) -> RepositoryResult; - /// Returns whether a durable bearer grant authorizes this upload. - async fn validate_upload_grant( + /// Claims a durable bearer grant, returning whether it authorized this + /// upload. + /// + /// Grants are single-use: a successful claim consumes the grant, so an + /// upload URL cannot be replayed within its TTL. Implementations must + /// make the claim itself the atomic step, so concurrent requests carrying + /// the same token cannot both succeed. + async fn claim_upload_grant( &self, token: &str, template_id: &str, From b6328520bfb49a6bf891293a8d9d76d66b18ad8f Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Tue, 28 Jul 2026 02:22:39 -0700 Subject: [PATCH 6/7] fix(template): address review findings and create WORKDIR directories WORKDIR now creates its directory in the build sandbox (mkdir -p, matching Docker), so a template built from a Dockerfile whose WORKDIR names a path absent from the base image no longer records a default cwd that does not exist in the guest. Review fixes, validated comment by comment against the code: - copy_plan: polynomial, segment-aware glob matching (was exponential and matched across '/'); the source archive is opened once and both passes read a capped reader, so GNU long-name/PAX records cannot allocate unboundedly; per-entry tar framing counts against the context budget; non-UTF-8 entry names are rejected. - COPY: extraction uses --no-overwrite-dir and no header is emitted for the destination root, so pre-existing directories keep their metadata while a missing destination is created with the requested --chown/--chmod; chown specs reject option-like parts. - Upload API: grants are verified up front but consumed only after the body is staged, so recoverable failures no longer burn the single-use URL; staging moves off the async worker; uploads time out via template_build.files_upload_timeout_secs; expiry math saturates. - Build-file stores: posixfs publishes via no-clobber hard_link, prunes grants by their own expiry, maps I/O errors instead of swallowing them, and refreshes archive mtimes on use; OSS materialize treats NotFound as a miss without a pre-check race. - Template API: filesHash on non-COPY/ADD steps, modes above 0o7777, and over-long COPY sources are rejected with 400s. - Catalog: alias rebinds take the previous owner's record lock (alias before record, documented); clearing a moved alias is guarded by the alias value on both backends; added a failure-path publish test. - Builder: a spec references at most 32 distinct archives, bounded in total by template_build.files_max_build_context_mib. - Config: public_base_url is validated at startup and documented as required behind TLS termination (upload URLs carry a bearer token); empty env values mean unset; new knobs are bounded at load time. Deferred (tracked in review replies): per-template archive namespacing and template-scoped authorization (blocked on the tenant model), conditional-write single-use grants on OSS, and alias reconciliation for duplicate listings. --- config/default.toml | 10 + docs/src/integration/e2b.md | 1 + src/api/build_files.rs | 123 +++- src/api/impls/template.rs | 4 +- src/api/impls/template_helpers.rs | 112 +++- src/api/proxy.rs | 18 + src/cfg.rs | 173 ++++++ .../repository/backends/oss/build_files.rs | 65 +- .../repository/backends/oss/repository.rs | 27 +- .../repository/backends/posixfs/backend.rs | 59 ++ .../backends/posixfs/build_files.rs | 309 +++++++++- .../repository/backends/posixfs/catalog.rs | 91 ++- src/snapshot/repository/build_files.rs | 34 +- src/template/build_spec.rs | 3 +- src/template/builder.rs | 62 ++ src/template/copy_plan.rs | 575 ++++++++++++++---- src/template/step_executor.rs | 168 ++++- 17 files changed, 1582 insertions(+), 252 deletions(-) diff --git a/config/default.toml b/config/default.toml index a058f347..0c8555a0 100644 --- a/config/default.toml +++ b/config/default.toml @@ -86,11 +86,21 @@ peer_discovery_refresh_interval_secs = 5 # files_max_upload_mib = 1024 # Maximum size one build-context archive may expand to once decompressed, in MiB. # files_max_context_mib = 4096 +# Cap on the combined on-disk size of all build-context archives one build spec +# may reference, in MiB. +# files_max_build_context_mib = 4096 # How long an issued upload URL stays valid, in seconds. # files_url_ttl_secs = 3600 +# How long one build-context upload request may run before the server gives up +# and responds 408, in seconds. +# files_upload_timeout_secs = 300 # Optional external base URL used when building upload URLs. Defaults to # "http://{Host header}" of the upload-link request, which matches # direct-node and bundled-gateway deployments. +# Any TLS-terminated, gateway-fronted, or multi-hop deployment MUST set this to +# the external origin clients reach: the fallback derives the URL from the +# request Host header with plain http, and that upload URL carries a bearer +# token in its query string. # public_base_url = "https://agentenv.example.com" [cluster] diff --git a/docs/src/integration/e2b.md b/docs/src/integration/e2b.md index ba1a7a23..f767fd20 100644 --- a/docs/src/integration/e2b.md +++ b/docs/src/integration/e2b.md @@ -123,6 +123,7 @@ Requirements and behavior notes: - Copied files are owned by `root:root` like Docker's `COPY` default. `COPY --chown=user:group` resolves names against the image's own `/etc/passwd` and `/etc/group` and applies only to the files the copy creates; an unknown user fails the build. - Write directory destinations with a trailing slash (`COPY app.py /opt/`). Docker's special case of copying a single file onto an existing directory named without a trailing slash (`COPY app.py /opt`) is not supported and fails the build with a clear error. - Rebuilding an existing alias is allowed: the alias keeps pointing at the previous template while the new build runs and moves to the new template when the build commits (E2B semantics). The previous template stays addressable by ID. A failed rebuild leaves the alias untouched. +- Any TLS-terminated, gateway-fronted, or multi-hop deployment must set `template_build.public_base_url` (or `AENV_TEMPLATE_BUILD_PUBLIC_BASE_URL`) to the external origin clients reach, for example `https://agentenv.example.com`. When it is unset, the upload URL is derived from the request `Host` header with plain `http`, and because that URL carries a bearer token in its query string the upload either fails against an HTTPS-only listener or sends the token and the whole build context in cleartext. Direct-node and bundled-gateway deployments can keep the default. ## E2B CLI diff --git a/src/api/build_files.rs b/src/api/build_files.rs index 9e0c3798..28d80d43 100644 --- a/src/api/build_files.rs +++ b/src/api/build_files.rs @@ -6,6 +6,8 @@ //! therefore the credential, and this route stays outside the generated //! router so the archive can stream to disk instead of buffering in memory. +use std::time::Duration; + use axum::extract::{Path, Request, State}; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; @@ -92,15 +94,16 @@ where ); }; - // Claiming consumes the grant, so an upload URL works exactly once. + // Verification does not consume the grant, so an upload that fails before + // the archive is stored can be retried with the same URL. let now_unix = chrono::Utc::now().timestamp(); let authorized = match store - .claim_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) + .verify_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) .await { Ok(authorized) => authorized, Err(error) => { - warn!(error = %error, "failed to claim build-file upload grant"); + warn!(error = %error, "failed to verify build-file upload grant"); return error_response( StatusCode::INTERNAL_SERVER_ERROR, "failed to validate upload grant", @@ -118,16 +121,30 @@ where .template_build .files_max_upload_mib .saturating_mul(1024 * 1024); + let upload_timeout = Duration::from_secs( + ConfigManager::global_config() + .template_build + .files_upload_timeout_secs, + ); - let staged = match tempfile::NamedTempFile::new() { - Ok(staged) => staged, - Err(error) => { + // `staged` is the drop guard that removes the staging file on every early + // return below, so it must stay bound for the rest of the handler. + let staged = match tokio::task::spawn_blocking(tempfile::NamedTempFile::new).await { + Ok(Ok(staged)) => staged, + Ok(Err(error)) => { warn!(error = %error, "failed to create staging file for build archive"); return error_response( StatusCode::INTERNAL_SERVER_ERROR, "failed to stage build archive", ); } + Err(error) => { + warn!(error = %error, "failed to join staging file creation for build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + ); + } }; let staged_path = staged.path().to_path_buf(); @@ -142,43 +159,87 @@ where } }; - let mut total: u64 = 0; - let mut stream = request.into_body().into_data_stream(); - while let Some(chunk) = stream.next().await { - let chunk = match chunk { - Ok(chunk) => chunk, - Err(error) => { - debug!(error = %error, "build archive upload stream aborted"); - return error_response( - StatusCode::BAD_REQUEST, - "failed to read the uploaded archive body", - ); + let consume_body = async { + let mut total: u64 = 0; + let mut stream = request.into_body().into_data_stream(); + while let Some(chunk) = stream.next().await { + let chunk = match chunk { + Ok(chunk) => chunk, + Err(error) => { + debug!(error = %error, "build archive upload stream aborted"); + return Err(error_response( + StatusCode::BAD_REQUEST, + "failed to read the uploaded archive body", + )); + } + }; + total += chunk.len() as u64; + if total > max_bytes { + return Err(error_response( + StatusCode::PAYLOAD_TOO_LARGE, + format!("build archive exceeds the configured limit of {max_bytes} bytes"), + )); } - }; - total += chunk.len() as u64; - if total > max_bytes { + if let Err(error) = file.write_all(&chunk).await { + warn!(error = %error, "failed to write staged build archive"); + return Err(error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + )); + } + } + if let Err(error) = file.flush().await { + warn!(error = %error, "failed to flush staged build archive"); + return Err(error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to stage build archive", + )); + } + Ok(total) + }; + + let total = match tokio::time::timeout(upload_timeout, consume_body).await { + Ok(Ok(total)) => total, + Ok(Err(response)) => return response, + Err(_) => { + debug!(template_id, hash, "build archive upload timed out"); return error_response( - StatusCode::PAYLOAD_TOO_LARGE, - format!("build archive exceeds the configured limit of {max_bytes} bytes"), + StatusCode::REQUEST_TIMEOUT, + format!( + "build archive upload did not complete within {} seconds", + upload_timeout.as_secs() + ), ); } - if let Err(error) = file.write_all(&chunk).await { - warn!(error = %error, "failed to write staged build archive"); + }; + drop(file); + + // Consuming the grant here keeps a failed upload retryable while the + // atomic remove/delete still picks a single winner among concurrent + // replays. `now_unix` is the timestamp taken before the body was read, so + // a slow but authorized upload is not rejected for aging past the TTL. + let claimed = match store + .claim_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) + .await + { + Ok(claimed) => claimed, + Err(error) => { + warn!(error = %error, "failed to claim build-file upload grant"); return error_response( StatusCode::INTERNAL_SERVER_ERROR, - "failed to stage build archive", + "failed to validate upload grant", ); } - } - if let Err(error) = file.flush().await { - warn!(error = %error, "failed to flush staged build archive"); + }; + if !claimed { return error_response( - StatusCode::INTERNAL_SERVER_ERROR, - "failed to stage build archive", + StatusCode::UNAUTHORIZED, + "upload grant is invalid, expired, or already used; request a fresh upload link", ); } - drop(file); + // `hash` is the cache key the SDK computed for this build context, not a + // digest of the received bytes that the server verified. if let Err(error) = store.import(&hash, &staged_path).await { warn!(error = %error, hash, "failed to import build archive"); return error_response( diff --git a/src/api/impls/template.rs b/src/api/impls/template.rs index 3537fecb..3c8727b5 100644 --- a/src/api/impls/template.rs +++ b/src/api/impls/template.rs @@ -376,7 +376,9 @@ impl Templates<()> for ApiImpl { } }; let config = &crate::cfg::ConfigManager::global_config().template_build; - let expires = chrono::Utc::now().timestamp() + config.files_url_ttl_secs as i64; + let expires = chrono::Utc::now() + .timestamp() + .saturating_add(i64::try_from(config.files_url_ttl_secs).unwrap_or(i64::MAX)); let token = match store.create_upload_grant(template_id, hash, expires).await { Ok(token) => token, Err(err) => { diff --git a/src/api/impls/template_helpers.rs b/src/api/impls/template_helpers.rs index 86c9eeb7..3bd4c3b0 100644 --- a/src/api/impls/template_helpers.rs +++ b/src/api/impls/template_helpers.rs @@ -124,12 +124,32 @@ pub(super) fn template_build_start_base_source( } } +/// Upper bound on a COPY/ADD source pattern. The pattern is matched against +/// every entry name of an uploaded archive, so its length directly bounds the +/// per-entry matching cost. +const MAX_COPY_SRC_BYTES: usize = 4096; + fn apply_e2b_template_step( mut spec: TemplateBuildSpec, step: &models::TemplateStep, ) -> Result { let args = step.args.as_deref().unwrap_or_default(); - match step.r_type.to_ascii_uppercase().as_str() { + let step_type = step.r_type.to_ascii_uppercase(); + let carries_files_hash = step + .files_hash + .as_deref() + .map(str::trim) + .is_some_and(|hash| !hash.is_empty()); + if carries_files_hash && !matches!(step_type.as_str(), "COPY" | "ADD") { + return Err(models::Error::new( + 400, + format!( + "{} template step must not carry a filesHash; only COPY and ADD consume build context archives", + step.r_type + ), + )); + } + match step_type.as_str() { "RUN" => { let Some(cmd) = args.first().filter(|cmd| !cmd.trim().is_empty()) else { return Err(models::Error::new( @@ -238,6 +258,15 @@ fn apply_e2b_template_step( format!("{} template step requires a source argument", step.r_type), ) })?; + if src.len() > MAX_COPY_SRC_BYTES { + return Err(models::Error::new( + 400, + format!( + "{} template step source argument exceeds {MAX_COPY_SRC_BYTES} bytes", + step.r_type + ), + )); + } let dest = args .get(1) .map(|value| value.trim()) @@ -261,7 +290,7 @@ fn apply_e2b_template_step( .map(|value| value.trim()) .filter(|value| !value.is_empty()) .map(|value| { - u32::from_str_radix(value, 8).map_err(|_| { + let mode = u32::from_str_radix(value, 8).map_err(|_| { models::Error::new( 400, format!( @@ -269,7 +298,17 @@ fn apply_e2b_template_step( step.r_type ), ) - }) + })?; + if mode > 0o7777 { + return Err(models::Error::new( + 400, + format!( + "{} template step mode '{value}' exceeds the maximum octal mode 7777", + step.r_type + ), + )); + } + Ok(mode) }) .transpose()?; spec = spec.copy(src, dest, files_hash, user, mode); @@ -287,9 +326,31 @@ fn apply_e2b_template_step( #[cfg(test)] mod tests { - use super::{template_build_start_base_source, TemplateBuildStartBaseSource}; + use super::{ + apply_e2b_template_step, template_build_start_base_source, TemplateBuildStartBaseSource, + MAX_COPY_SRC_BYTES, + }; + use crate::template::TemplateBuildSpec; use agentenv_http_server::models; + const HASH: &str = "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"; + + fn step(r_type: &str, args: &[&str]) -> models::TemplateStep { + let mut step = models::TemplateStep::new(r_type.to_string()); + step.args = Some(args.iter().map(|arg| (*arg).to_string()).collect()); + step + } + + fn copy_step(args: &[&str]) -> models::TemplateStep { + let mut step = step("COPY", args); + step.files_hash = Some(HASH.to_string()); + step + } + + fn apply(step: &models::TemplateStep) -> Result { + apply_e2b_template_step(TemplateBuildSpec::new(), step) + } + #[test] fn start_base_source_defaults_when_not_specified() { let body = models::TemplateBuildStartV2::new(); @@ -324,4 +385,47 @@ mod tests { ) ); } + + #[test] + fn step_type_is_matched_case_insensitively() { + apply(&step("workdir", &["/app"])).expect("lowercase step type should apply"); + } + + #[test] + fn files_hash_outside_copy_and_add_is_rejected() { + let mut run = step("RUN", &["echo hi"]); + run.files_hash = Some(HASH.to_string()); + + let err = apply(&run).expect_err("a RUN step must not carry a filesHash"); + assert_eq!(err.code, 400); + assert!(err.message.contains("filesHash"), "{}", err.message); + + // A blank filesHash stays acceptable: the SDK omits it as an empty + // string for non-COPY steps. + run.files_hash = Some(" ".to_string()); + apply(&run).expect("a blank filesHash should be ignored"); + } + + #[test] + fn copy_step_rejects_out_of_range_mode() { + apply(©_step(&["src", "/dest", "", "0755"])).expect("a valid mode should apply"); + + let err = apply(©_step(&["src", "/dest", "", "10000"])) + .expect_err("a mode above 7777 should be rejected"); + assert_eq!(err.code, 400); + assert!(err.message.contains("10000"), "{}", err.message); + } + + #[test] + fn copy_step_rejects_an_oversized_source_pattern() { + let oversized = "a".repeat(MAX_COPY_SRC_BYTES + 1); + let err = apply(©_step(&[oversized.as_str(), "/dest"])) + .expect_err("an oversized source should be rejected"); + assert_eq!(err.code, 400); + assert!( + err.message.contains(&MAX_COPY_SRC_BYTES.to_string()), + "{}", + err.message + ); + } } diff --git a/src/api/proxy.rs b/src/api/proxy.rs index 80710444..8472ee28 100644 --- a/src/api/proxy.rs +++ b/src/api/proxy.rs @@ -2024,6 +2024,24 @@ mod tests { .unwrap(); assert_eq!(response.status(), StatusCode::NOT_FOUND); + let content_type = response + .headers() + .get(axum::http::header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .to_owned(); + assert!( + content_type.starts_with("application/json"), + "unexpected content-type: {content_type}" + ); + let body = response.into_body().collect().await.unwrap().to_bytes(); + let payload: Value = serde_json::from_slice(&body).unwrap(); + assert_eq!(payload["code"], 404); + let message = payload["message"].as_str().unwrap(); + assert!( + message.contains("route not found: GET /nonexistent/path"), + "unexpected message: {message}" + ); } #[tokio::test] diff --git a/src/cfg.rs b/src/cfg.rs index d30f05d3..d525f25a 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -490,13 +490,26 @@ pub struct TemplateBuildConfig { /// the node that runs the build. #[config(default = 4096u64)] pub files_max_context_mib: u64, + /// Cap on the combined on-disk size of all build-context archives one + /// build spec may reference, in MiB. + #[config(default = 4096u64)] + pub files_max_build_context_mib: u64, /// How long an issued upload URL stays valid, in seconds. #[config(default = 3600u64)] pub files_url_ttl_secs: u64, + /// How long one build-context upload request may run before the server + /// gives up and responds 408, in seconds. + #[config(default = 300u64)] + pub files_upload_timeout_secs: u64, /// Optional external base URL (e.g. "https://agentenv.example.com") used /// when building upload URLs. When unset, upload URLs reuse the Host /// header of the upload-link request with plain http, which matches /// direct-node and bundled-gateway deployments. + /// + /// Any TLS-terminated, gateway-fronted, or multi-hop deployment MUST set + /// this to the external origin clients reach: the fallback derives the URL + /// from the request Host header with plain http, and that upload URL + /// carries a bearer token in its query string. #[config(env = "AENV_TEMPLATE_BUILD_PUBLIC_BASE_URL", parse_env = parse_trimmed_string)] pub public_base_url: Option, } @@ -776,6 +789,17 @@ impl AppConfig { self.cluster.normalize(); self.sandbox_proxy.normalize()?; + // An env var exported empty means unset, matching the custom-extension + // URL handling; validation and the upload-URL builder then agree on + // the exact value in use. + self.template_build.public_base_url = self + .template_build + .public_base_url + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_owned); + Ok(()) } @@ -804,6 +828,64 @@ impl AppConfig { } self.validate_memory_snapshot_background_download()?; self.validate_overlaybd_global_config_paths()?; + self.validate_template_build()?; + Ok(()) + } + + /// Reject template build-context settings that would only fail later, at + /// upload-link time: a base URL that cannot produce a usable upload URL, + /// or a TTL that makes the `now + ttl` expiry arithmetic overflow or the + /// grant effectively unexpirable. + fn validate_template_build(&self) -> Result<()> { + // 7 days. Upload grants are single-use credentials in a query string, + // so a longer window is always a misconfiguration. + const MAX_URL_TTL_SECS: u64 = 604_800; + let cfg = &self.template_build; + if cfg.files_url_ttl_secs == 0 { + bail!("template_build.files_url_ttl_secs must be > 0"); + } + if cfg.files_url_ttl_secs > MAX_URL_TTL_SECS { + bail!( + "template_build.files_url_ttl_secs must be <= {MAX_URL_TTL_SECS} (got {})", + cfg.files_url_ttl_secs + ); + } + if cfg.files_upload_timeout_secs == 0 { + bail!("template_build.files_upload_timeout_secs must be > 0"); + } + // An upload slower than the grant TTL would stage the whole body and + // then lose the grant to expiry-based pruning at claim time. + if cfg.files_upload_timeout_secs > cfg.files_url_ttl_secs { + bail!( + "template_build.files_upload_timeout_secs ({}) must be <= \ + files_url_ttl_secs ({})", + cfg.files_upload_timeout_secs, + cfg.files_url_ttl_secs + ); + } + if cfg.files_max_build_context_mib == 0 { + bail!("template_build.files_max_build_context_mib must be > 0"); + } + if let Some(base_url) = cfg.public_base_url.as_deref() { + let parsed = url::Url::parse(base_url).with_context(|| { + format!( + "invalid template_build.public_base_url {base_url:?}: must be an absolute \ + http/https URL" + ) + })?; + if !matches!(parsed.scheme(), "http" | "https") { + bail!( + "invalid template_build.public_base_url {base_url:?}: scheme must be http or \ + https" + ); + } + if parsed.query().is_some() { + bail!("invalid template_build.public_base_url {base_url:?}: must have no query"); + } + if parsed.fragment().is_some() { + bail!("invalid template_build.public_base_url {base_url:?}: must have no fragment"); + } + } Ok(()) } @@ -1197,6 +1279,97 @@ mod tests { assert!(config.validate().is_err()); } + #[test] + fn template_build_defaults_pass_validation() { + let config = AppConfig::default(); + assert_eq!(config.template_build.files_url_ttl_secs, 3600); + assert_eq!(config.template_build.files_upload_timeout_secs, 300); + assert_eq!(config.template_build.files_max_build_context_mib, 4096); + assert!(config.template_build.public_base_url.is_none()); + config.validate().expect("default config passes"); + } + + #[test] + fn validate_accepts_absolute_template_build_public_base_url() { + let mut config = AppConfig::default(); + config.template_build.public_base_url = Some("https://agentenv.example.com".to_string()); + + config.validate().expect("https base url passes"); + } + + #[test] + fn validate_rejects_template_build_public_base_url_with_query() { + let mut config = AppConfig::default(); + config.template_build.public_base_url = + Some("https://agentenv.example.com/?token=abc".to_string()); + + let err = config.validate().unwrap_err(); + let message = err.to_string(); + assert!(message.contains("public_base_url"), "{message}"); + assert!(message.contains("must have no query"), "{message}"); + } + + #[test] + fn validate_rejects_template_build_public_base_url_without_scheme() { + let mut config = AppConfig::default(); + config.template_build.public_base_url = Some("agentenv.example.com".to_string()); + + let err = config.validate().unwrap_err(); + assert!( + err.to_string().contains("public_base_url"), + "unexpected error: {err}" + ); + } + + #[test] + fn validate_bounds_template_build_files_url_ttl() { + let mut config = AppConfig::default(); + config.template_build.files_url_ttl_secs = 0; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("template_build.files_url_ttl_secs must be > 0"), + "unexpected error: {err}" + ); + + let mut config = AppConfig::default(); + config.template_build.files_url_ttl_secs = 604_801; + assert!(config.validate().is_err()); + + let mut config = AppConfig::default(); + config.template_build.files_url_ttl_secs = 604_800; + config.validate().expect("max ttl passes"); + } + + #[test] + fn validate_bounds_template_build_upload_timeout() { + let mut config = AppConfig::default(); + config.template_build.files_upload_timeout_secs = 0; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("template_build.files_upload_timeout_secs must be > 0"), + "unexpected error: {err}" + ); + + let mut config = AppConfig::default(); + config.template_build.files_upload_timeout_secs = 3601; + let err = config.validate().unwrap_err(); + assert!( + err.to_string().contains("must be <= files_url_ttl_secs"), + "unexpected error: {err}" + ); + + let mut config = AppConfig::default(); + config.template_build.files_max_build_context_mib = 0; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("template_build.files_max_build_context_mib must be > 0"), + "unexpected error: {err}" + ); + } + #[test] fn overlaybd_converter_cache_version_includes_tool_version() { let config = AppConfig::default(); diff --git a/src/snapshot/repository/backends/oss/build_files.rs b/src/snapshot/repository/backends/oss/build_files.rs index 4e9294a5..2ac034dd 100644 --- a/src/snapshot/repository/backends/oss/build_files.rs +++ b/src/snapshot/repository/backends/oss/build_files.rs @@ -39,6 +39,18 @@ impl OssTemplateBuildFileStore { is_valid_upload_token(token) .then(|| format!("{BUILD_FILES_PREFIX}/upload-grants/{token}.json")) } + + /// Reads a grant record, mapping an absent object to `None`. + async fn read_grant(&self, key: &str) -> RepositoryResult> { + let bytes = match self.client.get_bytes(key).await { + Ok(bytes) => bytes, + Err(error) if OssClient::is_not_found_error(&error) => return Ok(None), + Err(error) => return Err(RepositoryError::backend("read upload grant", error)), + }; + serde_json::from_slice(&bytes) + .map(Some) + .map_err(|error| RepositoryError::backend("parse upload grant", error)) + } } #[async_trait] @@ -53,8 +65,12 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()> { let key = Self::archive_key(hash)?; - // Archives are immutable: the hash addresses the content, so a repeat - // upload cannot change what an in-flight build reads. + // Archives are immutable so a repeat upload cannot change what an + // in-flight build reads. This fast path is not atomic against a + // concurrent import: the loser's bytes are dropped, and since the hash + // is a caller-supplied cache key rather than a verified digest, which + // racing upload wins is undefined — first-write-wins stability, not + // content authenticity. if self .client .exists(&key) @@ -75,20 +91,12 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { scratch_dir: &Path, ) -> RepositoryResult> { let key = Self::archive_key(hash)?; - if !self - .client - .exists(&key) - .await - .map_err(|error| RepositoryError::backend("check build archive", error))? - { - return Ok(None); - } let dest = scratch_dir.join(format!("{hash}.tar")); - self.client - .get_to_file(&key, &dest) - .await - .map_err(|error| RepositoryError::backend("download build archive", error))?; - Ok(Some(dest)) + match self.client.get_to_file(&key, &dest).await { + Ok(_) => Ok(Some(dest)), + Err(error) if OssClient::is_not_found_error(&error) => Ok(None), + Err(error) => Err(RepositoryError::backend("download build archive", error)), + } } async fn create_upload_grant( @@ -112,6 +120,25 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { Ok(token) } + async fn verify_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult { + let Some(key) = Self::grant_key(token) else { + return Ok(false); + }; + // Deliberately does not delete the object: verification must leave the + // upload URL usable for a retry. + let Some(grant) = self.read_grant(&key).await? else { + return Ok(false); + }; + Ok(grant.authorizes(template_id, hash, expires_unix, now_unix)) + } + async fn claim_upload_grant( &self, token: &str, @@ -123,13 +150,9 @@ impl TemplateBuildFileStore for OssTemplateBuildFileStore { let Some(key) = Self::grant_key(token) else { return Ok(false); }; - let bytes = match self.client.get_bytes(&key).await { - Ok(bytes) => bytes, - Err(error) if OssClient::is_not_found_error(&error) => return Ok(false), - Err(error) => return Err(RepositoryError::backend("read upload grant", error)), + let Some(grant) = self.read_grant(&key).await? else { + return Ok(false); }; - let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) - .map_err(|error| RepositoryError::backend("parse upload grant", error))?; if !grant.authorizes(template_id, hash, expires_unix, now_unix) { return Ok(false); } diff --git a/src/snapshot/repository/backends/oss/repository.rs b/src/snapshot/repository/backends/oss/repository.rs index f8ecbf92..b01d187a 100644 --- a/src/snapshot/repository/backends/oss/repository.rs +++ b/src/snapshot/repository/backends/oss/repository.rs @@ -304,7 +304,12 @@ impl SnapshotRepository for OssSnapshotRepository { // 5. Commit the record before moving the alias. This prevents an // alias from ever resolving to a snapshot whose catalog record - // has not been published yet. + // has not been published yet. The tradeoff is a crash window: + // dying after the record write but before `bind_alias` leaves a + // committed record whose `alias` field names an alias that still + // resolves to the previous snapshot, so readers of `record.alias` + // (listings) may observe the stale claim until the next rebind. + // Nothing reconciles that state automatically. let previous_record = self.read_record(id).await?; let previous_alias_target = if let Some(alias) = metadata.alias.as_ref() { match self.load_alias_target(alias.as_ref()).await? { @@ -342,7 +347,7 @@ impl SnapshotRepository for OssSnapshotRepository { .as_ref() .filter(|previous_id| *previous_id != id) { - if let Err(error) = self.clear_record_alias(previous_id).await { + if let Err(error) = self.clear_record_alias(previous_id, alias.as_ref()).await { warn!( alias = %alias, previous_snapshot_id = %previous_id, @@ -749,8 +754,11 @@ impl OssSnapshotRepository { return; } }; - // Do not overwrite a concurrent publisher that already moved the - // alias elsewhere. + // Skip when a concurrent publisher already moved the alias elsewhere. + // This only narrows the lost-update window: like `bind_alias`, the + // rollback cannot be atomic on a store without conditional writes, so a + // publisher that rebinds between this read and the write below is still + // clobbered. if current.as_ref() != Some(id) { return; } @@ -786,9 +794,16 @@ impl OssSnapshotRepository { /// Clears the alias field on the record that previously owned a rebound /// alias so template listings do not report the moved name twice. - async fn clear_record_alias(&self, id: &SnapshotId) -> RepositoryResult<()> { + /// + /// Only `moved_alias` is cleared; a previous owner that already claims a + /// different name keeps it. + async fn clear_record_alias(&self, id: &SnapshotId, moved_alias: &str) -> RepositoryResult<()> { if let Some(mut previous) = self.read_record(id).await? { - if previous.alias.is_some() { + let claims_moved_alias = previous + .alias + .as_ref() + .is_some_and(|alias| alias.as_ref() == moved_alias); + if claims_moved_alias { previous.alias = None; previous.updated_at_unix_ms = now_unix_ms(); self.write_record(&previous).await?; diff --git a/src/snapshot/repository/backends/posixfs/backend.rs b/src/snapshot/repository/backends/posixfs/backend.rs index 3e90fec3..713a4961 100644 --- a/src/snapshot/repository/backends/posixfs/backend.rs +++ b/src/snapshot/repository/backends/posixfs/backend.rs @@ -528,6 +528,65 @@ mod tests { ); } + #[tokio::test] + async fn failed_publish_keeps_previous_alias_and_removes_snapshot_dir() { + let tempdir = TempDir::new().expect("tempdir should exist"); + let repository = test_backend(tempdir.path()).repository(); + + let first_id = SnapshotId::generate(); + let local_artifacts = seed_built_snapshot(tempdir.path()); + repository + .publish( + sample_metadata(first_id.clone(), Some("rebind")), + local_artifacts, + ) + .await + .expect("first publish should work"); + + let second_id = SnapshotId::generate(); + let broken_artifacts = seed_built_snapshot(tempdir.path()); + // `import_built_artifacts` copies `vm_state.bin` first, so removing it + // fails the publish before any catalog state is committed. + fs::remove_file(&broken_artifacts.vm_state.path).expect("remove seeded vm state"); + repository + .publish( + sample_metadata(second_id.clone(), Some("rebind")), + broken_artifacts, + ) + .await + .expect_err("publish should fail when the vm state artifact is missing"); + + assert!( + !tempdir + .path() + .join("snapshots") + .join(second_id.to_string()) + .exists(), + "failed publish should not leave a snapshot directory behind" + ); + + let resolved = repository + .resolve_alias("rebind") + .await + .expect("resolve should work") + .expect("alias should still resolve"); + assert_eq!( + resolved, first_id, + "alias should stay bound to the previously committed snapshot" + ); + + let previous = repository + .get(first_id.to_string().as_str()) + .await + .expect("get should work") + .expect("previous snapshot should stay addressable by id"); + assert_eq!( + previous.alias.as_ref().map(ToString::to_string), + Some("rebind".to_string()), + "previous snapshot should keep the alias after a failed rebind" + ); + } + #[tokio::test] async fn create_keeps_existing_alias_until_new_build_commits() { let tempdir = TempDir::new().expect("tempdir should exist"); diff --git a/src/snapshot/repository/backends/posixfs/build_files.rs b/src/snapshot/repository/backends/posixfs/build_files.rs index a40298b7..4005ff12 100644 --- a/src/snapshot/repository/backends/posixfs/build_files.rs +++ b/src/snapshot/repository/backends/posixfs/build_files.rs @@ -64,15 +64,39 @@ impl PosixFsTemplateBuildFileStore { Self::prune_dir_older_than(root, "tar", cutoff); } - /// Removes upload grants older than the retention window. Runs + /// Removes upload grants that have passed their own `expires_unix`. Runs /// opportunistically whenever a new grant is written, so the grants /// directory stays bounded by upload-link traffic; failures only log. + /// + /// Pruning by the record rather than by mtime keeps grants alive for + /// exactly their TTL even when `template_build.files_url_ttl_secs` is + /// configured beyond the retention window. fn prune_expired_grants(root: &Path) { let cutoff = SystemTime::now() - BUILD_FILE_RETENTION; - Self::prune_dir_older_than(&Self::grants_dir(root), "json", cutoff); + let now_unix = chrono::Utc::now().timestamp(); + Self::prune_dir(&Self::grants_dir(root), "json", |path, modified| { + match fs::read(path) + .ok() + .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) + { + Some(grant) => grant.expires_unix < now_unix, + // Unparseable leftovers fall back to the mtime rule. + None => modified.is_some_and(|modified| modified < cutoff), + } + }); } fn prune_dir_older_than(dir: &Path, extension: &str, cutoff: SystemTime) { + Self::prune_dir(dir, extension, |_, modified| { + modified.is_some_and(|modified| modified < cutoff) + }); + } + + fn prune_dir( + dir: &Path, + extension: &str, + is_expired: impl Fn(&Path, Option) -> bool, + ) { let Ok(entries) = fs::read_dir(dir) else { return; }; @@ -81,12 +105,11 @@ impl PosixFsTemplateBuildFileStore { if path.extension().is_none_or(|ext| ext != extension) { continue; } - let expired = entry + let modified = entry .metadata() .and_then(|metadata| metadata.modified()) - .map(|modified| modified < cutoff) - .unwrap_or(false); - if expired { + .ok(); + if is_expired(&path, modified) { if let Err(error) = fs::remove_file(&path) { warn!( path = %path.display(), @@ -108,6 +131,36 @@ impl PosixFsTemplateBuildFileStore { is_valid_upload_token(token).then(|| Self::grants_dir(root).join(format!("{token}.json"))) } + /// Reads a grant record, mapping an absent file to `None`. + fn read_grant(path: &Path) -> RepositoryResult> { + let bytes = match fs::read(path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(RepositoryError::backend("read upload grant", error)), + }; + serde_json::from_slice(&bytes) + .map(Some) + .map_err(|error| RepositoryError::backend("parse upload grant", error)) + } + + /// Best-effort mtime refresh, so retention means "unused for the window" + /// and an archive a build is still reading stays outside the prune + /// horizon. Read-only repository mounts must keep working, so failures + /// only log. + fn touch(path: &Path) { + let refreshed = fs::File::options() + .write(true) + .open(path) + .and_then(|file| file.set_times(fs::FileTimes::new().set_modified(SystemTime::now()))); + if let Err(error) = refreshed { + debug!( + path = %path.display(), + error = %error, + "failed to refresh build archive mtime" + ); + } + } + fn write_grant( root: &Path, template_id: &str, @@ -161,9 +214,18 @@ impl PosixFsTemplateBuildFileStore { impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { async fn exists(&self, hash: &str) -> RepositoryResult { let path = self.archive_path(hash)?; - task::spawn_blocking(move || path.exists()) - .await - .map_err(|error| RepositoryError::backend("join build file exists task", error)) + task::spawn_blocking(move || -> RepositoryResult { + match fs::metadata(&path) { + Ok(_) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(RepositoryError::backend( + format!("stat build archive '{}'", path.display()), + error, + )), + } + }) + .await + .map_err(|error| RepositoryError::backend("join build file exists task", error))? } async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()> { @@ -179,17 +241,23 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { Self::ensure_root(&root)?; Self::prune_expired(&root); // Copy into the store filesystem first (the staged file usually - // lives on node-local tmp), then rename within the store directory - // so readers only ever observe complete archives. + // lives on node-local tmp), then link it into place within the + // store directory so readers only ever observe complete archives. let store_staged = root.join(format!(".import-{}.tmp", uuid::Uuid::new_v4())); fs::copy(&staged, &store_staged).map_err(|error| { let _ = fs::remove_file(&store_staged); RepositoryError::backend("copy build archive into store", error) })?; - fs::rename(&store_staged, &final_path).map_err(|error| { - let _ = fs::remove_file(&store_staged); - RepositoryError::backend("publish build archive", error) - }) + // Link rather than rename so a concurrent import cannot replace an + // archive a running build is already reading: the first writer + // wins and everyone else observes `AlreadyExists`. + let published = match fs::hard_link(&store_staged, &final_path) { + Ok(()) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(()), + Err(error) => Err(RepositoryError::backend("publish build archive", error)), + }; + let _ = fs::remove_file(&store_staged); + published }) .await .map_err(|error| RepositoryError::backend("join build file import task", error))? @@ -201,9 +269,21 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { _scratch_dir: &Path, ) -> RepositoryResult> { let path = self.archive_path(hash)?; - task::spawn_blocking(move || path.exists().then_some(path)) - .await - .map_err(|error| RepositoryError::backend("join build file materialize task", error)) + task::spawn_blocking(move || -> RepositoryResult> { + match fs::metadata(&path) { + Ok(_) => { + Self::touch(&path); + Ok(Some(path)) + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(error) => Err(RepositoryError::backend( + format!("stat build archive '{}'", path.display()), + error, + )), + } + }) + .await + .map_err(|error| RepositoryError::backend("join build file materialize task", error))? } async fn create_upload_grant( @@ -220,6 +300,31 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { .map_err(|error| RepositoryError::backend("join create upload grant task", error))? } + async fn verify_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult { + let Some(path) = Self::grant_path(&self.root, token) else { + return Ok(false); + }; + let template_id = template_id.to_string(); + let hash = hash.to_string(); + task::spawn_blocking(move || -> RepositoryResult { + // Reads only: the grant file must survive so an upload that fails + // before the archive is stored can be retried with the same URL. + let Some(grant) = Self::read_grant(&path)? else { + return Ok(false); + }; + Ok(grant.authorizes(&template_id, &hash, expires_unix, now_unix)) + }) + .await + .map_err(|error| RepositoryError::backend("join verify upload grant task", error))? + } + async fn claim_upload_grant( &self, token: &str, @@ -233,16 +338,10 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { }; let template_id = template_id.to_string(); let hash = hash.to_string(); - task::spawn_blocking(move || { - let bytes = match fs::read(&path) { - Ok(bytes) => bytes, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), - Err(error) => { - return Err(RepositoryError::backend("read upload grant", error)); - } + task::spawn_blocking(move || -> RepositoryResult { + let Some(grant) = Self::read_grant(&path)? else { + return Ok(false); }; - let grant: TemplateBuildUploadGrant = serde_json::from_slice(&bytes) - .map_err(|error| RepositoryError::backend("parse upload grant", error))?; if !grant.authorizes(&template_id, &hash, expires_unix, now_unix) { return Ok(false); } @@ -422,6 +521,29 @@ mod tests { .await .expect("repeat import should be accepted"); + // Two imports racing for a hash neither has stored yet must both + // succeed; the loser's hard link hits AlreadyExists and is dropped. + // A fresh hash keeps both calls off the exists() fast path. + const FRESH_HASH: &str = "f00ff00ff00ff00ff00ff00ff00ff00f"; + let concurrent = tempdir.path().join("concurrent.tar"); + fs::write(&concurrent, b"concurrent").expect("write concurrent"); + let (left, right) = tokio::join!( + store.import(FRESH_HASH, &replacement), + store.import(FRESH_HASH, &concurrent) + ); + left.expect("concurrent import should be accepted"); + right.expect("concurrent import should be accepted"); + let winner = store + .materialize(FRESH_HASH, tempdir.path()) + .await + .expect("materialize should work") + .expect("archive should exist"); + let winner_bytes = fs::read(winner).expect("read winner"); + assert!( + winner_bytes == b"replaced" || winner_bytes == b"concurrent", + "stored bytes must come from one of the racing imports" + ); + let materialized = store .materialize(HASH, tempdir.path()) .await @@ -432,5 +554,138 @@ mod tests { b"original", "a stored archive must never be replaced underneath a build" ); + + let leftovers = fs::read_dir(tempdir.path().join("template-build-files")) + .expect("read store dir") + .flatten() + .filter(|entry| entry.file_name().to_string_lossy().starts_with(".import-")) + .count(); + assert_eq!(leftovers, 0, "import must not leak staging files"); + } + + #[tokio::test] + async fn materialize_refreshes_the_archive_mtime() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + let staged = staged_file(tempdir.path(), b"tar-bytes"); + store.import(HASH, &staged).await.expect("import"); + + let archive = tempdir + .path() + .join("template-build-files") + .join(format!("{HASH}.tar")); + let stale = SystemTime::now() - BUILD_FILE_RETENTION - Duration::from_secs(60); + let file = fs::File::options() + .write(true) + .open(&archive) + .expect("open archive"); + file.set_times(fs::FileTimes::new().set_modified(stale)) + .expect("set stale mtime"); + drop(file); + + store + .materialize(HASH, tempdir.path()) + .await + .expect("materialize should work") + .expect("archive should exist"); + + let modified = fs::metadata(&archive) + .and_then(|metadata| metadata.modified()) + .expect("read archive mtime"); + assert!( + modified > stale, + "materializing an archive must keep it outside the prune horizon" + ); + } + + #[tokio::test] + async fn verifying_a_grant_does_not_consume_it() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let token = store + .create_upload_grant("template", HASH, 1000) + .await + .expect("grant should be created"); + + for _ in 0..2 { + assert!( + store + .verify_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("verification should work"), + "verification must not consume the grant" + ); + } + assert!(!store + .verify_upload_grant(&token, "other", HASH, 1000, 999) + .await + .expect("mismatched grant should be rejected")); + + // An upload that failed after verification can still be retried. + assert!(store + .claim_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("claim should succeed")); + assert!( + !store + .verify_upload_grant(&token, "template", HASH, 1000, 999) + .await + .expect("verification should work"), + "a consumed grant must no longer verify" + ); + } + + #[tokio::test] + async fn concurrent_claims_pick_a_single_winner() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + let token = store + .create_upload_grant("template", HASH, 1000) + .await + .expect("grant should be created"); + + let (left, right) = tokio::join!( + store.claim_upload_grant(&token, "template", HASH, 1000, 999), + store.claim_upload_grant(&token, "template", HASH, 1000, 999) + ); + let claims = [ + left.expect("claim should work"), + right.expect("claim should work"), + ]; + assert_eq!( + claims.iter().filter(|claimed| **claimed).count(), + 1, + "exactly one concurrent claim may win" + ); + } + + #[tokio::test] + async fn grants_are_pruned_once_their_own_expiry_passes() { + let tempdir = TempDir::new().expect("tempdir"); + let store = PosixFsTemplateBuildFileStore::new(tempdir.path()); + + // Expired long ago in grant terms, but freshly written on disk, so the + // mtime rule alone would keep it for the whole retention window. + let expired_token = store + .create_upload_grant("template", HASH, 1000) + .await + .expect("grant should be created"); + let expired_path = tempdir + .path() + .join("template-build-files") + .join("upload-grants") + .join(format!("{expired_token}.json")); + assert!(expired_path.exists()); + + store + .create_upload_grant("template", HASH, i64::MAX) + .await + .expect("new grant should be created"); + + assert!( + !expired_path.exists(), + "a grant past its own expiry should be pruned" + ); } } diff --git a/src/snapshot/repository/backends/posixfs/catalog.rs b/src/snapshot/repository/backends/posixfs/catalog.rs index ae122637..e617fdc2 100644 --- a/src/snapshot/repository/backends/posixfs/catalog.rs +++ b/src/snapshot/repository/backends/posixfs/catalog.rs @@ -101,7 +101,11 @@ impl PosixFsCatalogStore { store.write_committed_record_unlocked(&record)?; // `write_json` uses an atomic rename. Keeping this as the final // fallible operation means a failed rebuild leaves the old - // alias binding untouched. + // alias binding untouched. The tradeoff is a crash window: dying + // after the record write but before the alias write leaves a + // committed record whose `alias` field names an alias that still + // resolves to the previous snapshot, so readers of `record.alias` + // (listings) may observe the stale claim until the next rebind. store.write_json(&alias_path, &snapshot_id)?; if let Some(existing) = existing.filter(|existing| existing != &snapshot_id) { @@ -109,29 +113,7 @@ impl PosixFsCatalogStore { // sandboxes and explicit id references keep working. Alias // metadata cleanup is best effort because the binding has // already moved successfully. - match store.load_record_by_id_unlocked(&existing) { - Ok(Some(mut previous)) => { - previous.alias = None; - previous.updated_at_unix_ms = now; - if let Err(error) = store.write_record_unlocked(&previous) { - warn!( - alias = %alias, - previous_snapshot_id = %existing, - error = %error, - "failed to clear previous snapshot alias metadata" - ); - } - } - Ok(None) => {} - Err(error) => { - warn!( - alias = %alias, - previous_snapshot_id = %existing, - error = %error, - "failed to load previous snapshot alias metadata" - ); - } - } + store.clear_moved_alias_on_previous_record(&existing, alias.as_ref(), now); } Ok(record) }) @@ -403,6 +385,67 @@ impl PosixFsCatalogStore { self.write_record_unlocked(&record) } + /// Clears `moved_alias` from the record that owned it before a rebind. + /// + /// Best effort: the alias binding has already moved, so a failure here only + /// leaves stale alias metadata on the previous owner's record. + fn clear_moved_alias_on_previous_record( + &self, + previous_id: &SnapshotId, + moved_alias: &str, + now: i64, + ) { + // Lock order is alias lock first, then record lock; nothing takes them + // in the reverse order today. + let _guard = match self.acquire_record_lock(previous_id) { + Ok(guard) => guard, + Err(error) => { + warn!( + alias = %moved_alias, + previous_snapshot_id = %previous_id, + error = %error, + "failed to lock previous snapshot record for alias cleanup" + ); + return; + } + }; + + let mut previous = match self.load_record_by_id_unlocked(previous_id) { + Ok(Some(previous)) => previous, + Ok(None) => return, + Err(error) => { + warn!( + alias = %moved_alias, + previous_snapshot_id = %previous_id, + error = %error, + "failed to load previous snapshot alias metadata" + ); + return; + } + }; + + // Only clear the alias this publish actually moved; the previous owner + // may already claim a different name. + let claims_moved_alias = previous + .alias + .as_ref() + .is_some_and(|alias| alias.as_ref() == moved_alias); + if !claims_moved_alias { + return; + } + + previous.alias = None; + previous.updated_at_unix_ms = now; + if let Err(error) = self.write_record_unlocked(&previous) { + warn!( + alias = %moved_alias, + previous_snapshot_id = %previous_id, + error = %error, + "failed to clear previous snapshot alias metadata" + ); + } + } + fn read_json(&self, path: &Path) -> RepositoryResult where T: DeserializeOwned, diff --git a/src/snapshot/repository/build_files.rs b/src/snapshot/repository/build_files.rs index 9395129a..163442a9 100644 --- a/src/snapshot/repository/build_files.rs +++ b/src/snapshot/repository/build_files.rs @@ -66,10 +66,12 @@ pub trait TemplateBuildFileStore: Send + Sync { /// Imports a fully written local file as the archive for `hash`. /// /// Implementations must publish atomically: concurrent readers never - /// observe a partially imported archive. Archives are content-addressed - /// and therefore immutable — importing a hash that is already stored - /// keeps the stored archive, so an in-flight build can never observe its - /// build context change underneath it. + /// observe a partially imported archive. `hash` is the cache key supplied + /// by the authenticated caller, not a digest the store verifies, so + /// immutability here means first-write-wins stability rather than content + /// authenticity: importing a hash that is already stored keeps the stored + /// archive, so an in-flight build can never observe its build context + /// change underneath it. async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; /// Materializes the archive for `hash` as a node-local file. @@ -93,13 +95,33 @@ pub trait TemplateBuildFileStore: Send + Sync { expires_unix: i64, ) -> RepositoryResult; + /// Verifies a durable bearer grant without consuming it, returning + /// whether it authorizes this upload. + /// + /// Verification never removes the grant, so a request that fails before + /// the archive is stored can be retried with the same upload URL. Callers + /// must still `claim_upload_grant` before publishing the archive. + async fn verify_upload_grant( + &self, + token: &str, + template_id: &str, + hash: &str, + expires_unix: i64, + now_unix: i64, + ) -> RepositoryResult; + /// Claims a durable bearer grant, returning whether it authorized this /// upload. /// /// Grants are single-use: a successful claim consumes the grant, so an /// upload URL cannot be replayed within its TTL. Implementations must - /// make the claim itself the atomic step, so concurrent requests carrying - /// the same token cannot both succeed. + /// make the claim itself the atomic step wherever the backend offers an + /// atomic primitive (a POSIX filesystem does, via rename/unlink), so + /// concurrent requests carrying the same token cannot both succeed. + /// S3-compatible backends have no conditional delete and therefore + /// degrade to best-effort single-use within the grant TTL; archive + /// immutability is what keeps a lost race from mattering, since the + /// competing uploads can only publish the same content-addressed archive. async fn claim_upload_grant( &self, token: &str, diff --git a/src/template/build_spec.rs b/src/template/build_spec.rs index 1faf9a48..3d5957f2 100644 --- a/src/template/build_spec.rs +++ b/src/template/build_spec.rs @@ -230,10 +230,11 @@ impl TemplateBuildSpec { /// Returns the distinct build-context archive hashes referenced by COPY steps. pub(crate) fn referenced_build_file_hashes(steps: &[TemplateBuildStep]) -> Vec { + let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); let mut hashes: Vec = Vec::new(); for step in steps { if let TemplateBuildStepKind::Copy { files_hash, .. } = &step.kind { - if !hashes.iter().any(|existing| existing == files_hash) { + if seen.insert(files_hash.as_str()) { hashes.push(files_hash.clone()); } } diff --git a/src/template/builder.rs b/src/template/builder.rs index 54a2d290..ea8cf059 100644 --- a/src/template/builder.rs +++ b/src/template/builder.rs @@ -18,6 +18,9 @@ use crate::snapshot::{ }; use crate::types::SandboxResources; +/// Bounds the node disk cost of one build spec. +const MAX_BUILD_ARCHIVES: usize = 32; + #[derive(Clone)] /// Coordinates template-builder flows over committed snapshots. pub struct TemplateBuilder { @@ -164,6 +167,14 @@ impl TemplateBuilder { if hashes.is_empty() { return Ok(std::collections::HashMap::new()); } + if hashes.len() > MAX_BUILD_ARCHIVES { + return Err(TemplateBuildError::invalid_input(format!( + "template build references {} distinct build context archives, which exceeds the \ + limit of {MAX_BUILD_ARCHIVES}", + hashes.len() + )) + .into()); + } let Some(store) = snapshot_manager.template_build_files() else { return Err(TemplateBuildError::invalid_input( @@ -177,10 +188,34 @@ impl TemplateBuilder { TemplateBuildError::with_source("create build archive scratch dir", error) })?; + let max_context_mib = ConfigManager::global_config() + .template_build + .files_max_build_context_mib; + let max_total_bytes = max_context_mib.saturating_mul(1024 * 1024); + let mut archives = std::collections::HashMap::new(); + let mut total_bytes: u64 = 0; for hash in hashes { match store.materialize(&hash, &scratch).await { Ok(Some(path)) => { + let size = tokio::fs::metadata(&path) + .await + .map_err(|error| { + TemplateBuildError::with_source( + format!("read build context archive '{hash}' metadata"), + error, + ) + })? + .len(); + total_bytes = total_bytes.saturating_add(size); + if total_bytes > max_total_bytes { + return Err(TemplateBuildError::invalid_input(format!( + "template build context archives total at least {total_bytes} bytes, \ + which exceeds the template_build.files_max_build_context_mib limit of \ + {max_context_mib} MiB" + )) + .into()); + } archives.insert(hash, path); } Ok(None) => { @@ -431,6 +466,33 @@ mod tests { )); } + #[tokio::test] + async fn materialize_build_archives_rejects_too_many_archives() { + let tempdir = TempDir::new().expect("tempdir"); + let (manager, snapshot_manager) = test_parts(tempdir.path()); + let runnable = RunnableSnapshot::mock(); + let mut spec = TemplateBuildSpec::new(); + for index in 0..=MAX_BUILD_ARCHIVES { + spec = spec.copy("./src", "/app", format!("hash-{index}"), None, None); + } + let context = manager + .prepare_snapshot_base_context( + &spec, + crate::snapshot::SnapshotId::generate(), + &runnable, + ) + .expect("snapshot-base preparation should succeed"); + + let err = TemplateBuilder::materialize_build_archives(&snapshot_manager, &context) + .await + .expect_err("too many referenced archives should be rejected"); + + assert!(matches!( + err, + crate::template::TemplatePipelineError::Build(TemplateBuildError::InvalidInput { .. }) + )); + } + #[tokio::test] async fn build_snapshot_base_rejects_overriding_rootfs_base() { let manager = TemplateBuilder::new(); diff --git a/src/template/copy_plan.rs b/src/template/copy_plan.rs index 1cb00c82..27feae23 100644 --- a/src/template/copy_plan.rs +++ b/src/template/copy_plan.rs @@ -8,10 +8,15 @@ //! //! The rewrite runs in two passes so an archive is never held in memory: the //! first pass indexes entry paths to compute the mapping, the second streams -//! each entry's bytes straight into the rewritten archive. +//! each entry's bytes straight into the rewritten archive. Both passes read +//! from a single open file handle so the source cannot be replaced or unlinked +//! between them. //! //! Ownership is written into the rewritten headers rather than applied with a //! post-extract `chown`, so a copy can only ever change the files it creates. +//! For the same reason the destination root itself never gets an archive +//! entry: `tar -xp` restores mode and ownership onto directory members that +//! already exist. use std::fs::File; use std::io::{BufReader, Read, Seek, SeekFrom, Write}; @@ -54,6 +59,16 @@ pub(crate) struct CopyPlan { pub(crate) entry_count: usize, /// Total file bytes written to the rewritten archive. pub(crate) total_bytes: u64, + /// Resolved absolute guest path of the copy destination root. + pub(crate) dest_root: String, + /// Whether a directory entry for `dest_root` itself was dropped. When set, + /// the guest has to create that directory (with the requested ownership + /// and mode) before extraction, but only if it does not already exist. + pub(crate) skipped_dest_root: bool, + /// Whether the copy treats `dest_root` as a directory. Archives without a + /// directory member for the root (file-only uploads) still need the guest + /// to create a missing destination with the requested metadata. + pub(crate) dest_is_dir: bool, } /// One archive entry as seen by the indexing pass. @@ -67,55 +82,147 @@ fn is_glob_pattern(src: &str) -> bool { src.contains(['*', '?', '[']) } +/// One member of a `[...]` character class. +enum ClassItem { + Char(char), + Range(char, char), +} + +/// One matchable unit inside a single path segment of a glob pattern. +enum GlobToken { + Star, + Any, + Literal(char), + Class { + negated: bool, + items: Vec, + }, +} + +/// Splits one pattern segment into tokens. An unterminated `[` is a literal. +fn tokenize_segment(pattern: &[char]) -> Vec { + let mut tokens = Vec::new(); + let mut i = 0; + while i < pattern.len() { + match pattern[i] { + '*' => { + tokens.push(GlobToken::Star); + i += 1; + } + '?' => { + tokens.push(GlobToken::Any); + i += 1; + } + '[' => match pattern[i + 1..].iter().position(|&c| c == ']') { + None => { + tokens.push(GlobToken::Literal('[')); + i += 1; + } + Some(end) => { + let class = &pattern[i + 1..i + 1 + end]; + let (negated, class) = match class.first() { + Some('!') | Some('^') => (true, &class[1..]), + _ => (false, class), + }; + let mut items = Vec::new(); + let mut j = 0; + while j < class.len() { + if j + 2 < class.len() && class[j + 1] == '-' { + items.push(ClassItem::Range(class[j], class[j + 2])); + j += 3; + } else { + items.push(ClassItem::Char(class[j])); + j += 1; + } + } + tokens.push(GlobToken::Class { negated, items }); + i += end + 2; + } + }, + c => { + tokens.push(GlobToken::Literal(c)); + i += 1; + } + } + } + tokens +} + +/// Whether a single-character token accepts `c`. +fn token_matches(token: &GlobToken, c: char) -> bool { + match token { + GlobToken::Star => false, + GlobToken::Any => true, + GlobToken::Literal(expected) => *expected == c, + GlobToken::Class { negated, items } => { + let hit = items.iter().any(|item| match item { + ClassItem::Char(ch) => *ch == c, + ClassItem::Range(low, high) => *low <= c && c <= *high, + }); + hit != *negated + } + } +} + +/// Matches one segment with a single backtrack point per `*`, which keeps the +/// worst case quadratic instead of the exponential blowup a naive recursive +/// matcher has on patterns such as `*a*a*a*a*b`. +fn match_segment(tokens: &[GlobToken], value: &[char]) -> bool { + let mut token_idx = 0usize; + let mut value_idx = 0usize; + let mut last_star: Option = None; + let mut last_star_value = 0usize; + + while value_idx < value.len() { + if token_idx < tokens.len() { + if matches!(tokens[token_idx], GlobToken::Star) { + last_star = Some(token_idx); + last_star_value = value_idx; + token_idx += 1; + continue; + } + if token_matches(&tokens[token_idx], value[value_idx]) { + token_idx += 1; + value_idx += 1; + continue; + } + } + // Mismatch: let the most recent `*` swallow one more character. + let Some(star_idx) = last_star else { + return false; + }; + token_idx = star_idx + 1; + last_star_value += 1; + value_idx = last_star_value; + } + + tokens[token_idx..] + .iter() + .all(|token| matches!(token, GlobToken::Star)) +} + /// Minimal fnmatch-style matcher covering `*`, `?` and `[...]` (no `**`), /// mirroring the Python `glob` patterns the SDK resolves client-side. +/// +/// Matching is segment-wise like Go's `path/filepath.Match`, which is what +/// Docker uses for `COPY` sources: none of the wildcards ever match `/`, so a +/// pattern and a value with different segment counts never match. fn glob_match(pattern: &str, value: &str) -> bool { - fn inner(pattern: &[char], value: &[char]) -> bool { - match pattern.split_first() { - None => value.is_empty(), - Some(('*', rest)) => (0..=value.len()).any(|skip| inner(rest, &value[skip..])), - Some(('?', rest)) => !value.is_empty() && inner(rest, &value[1..]), - Some(('[', rest)) => { - let Some(end) = rest.iter().position(|&c| c == ']') else { - // No closing bracket: treat '[' as a literal character. - return !value.is_empty() && value[0] == '[' && inner(rest, &value[1..]); - }; - let (class, after) = rest.split_at(end); - let after = &after[1..]; - let Some(&first) = value.first() else { + let mut pattern_segments = pattern.split('/'); + let mut value_segments = value.split('/'); + loop { + match (pattern_segments.next(), value_segments.next()) { + (None, None) => return true, + (Some(pattern_segment), Some(value_segment)) => { + let pattern_segment: Vec = pattern_segment.chars().collect(); + let value_segment: Vec = value_segment.chars().collect(); + if !match_segment(&tokenize_segment(&pattern_segment), &value_segment) { return false; - }; - let (negated, class) = match class.first() { - Some('!') | Some('^') => (true, &class[1..]), - _ => (false, class), - }; - let mut matched = false; - let mut i = 0; - while i < class.len() { - if i + 2 < class.len() && class[i + 1] == '-' { - if class[i] <= first && first <= class[i + 2] { - matched = true; - } - i += 3; - } else { - if class[i] == first { - matched = true; - } - i += 1; - } - } - if matched != negated { - inner(after, &value[1..]) - } else { - false } } - Some((&c, rest)) => !value.is_empty() && value[0] == c && inner(rest, &value[1..]), + _ => return false, } } - let pattern: Vec = pattern.chars().collect(); - let value: Vec = value.chars().collect(); - inner(&pattern, &value) } /// Normalizes a context-relative source pattern ("./a/b/" -> "a/b"). @@ -176,7 +283,15 @@ fn normalize_entry_path(raw: &Path) -> Result { for component in raw.components() { match component { std::path::Component::Normal(part) => { - parts.push(part.to_string_lossy().into_owned()); + // Lossy conversion would collapse distinct non-UTF-8 names + // onto one replacement-character name, silently overwriting. + let Some(part) = part.to_str() else { + bail!( + "non-UTF-8 path component in archive entry '{}'", + raw.display() + ); + }; + parts.push(part.to_string()); } std::path::Component::CurDir => {} other => bail!( @@ -192,24 +307,67 @@ fn normalize_entry_path(raw: &Path) -> Result { Ok(parts.join("/")) } -/// Opens the uploaded archive, transparently decompressing gzip. -fn open_archive(source_tar: &Path) -> Result>> { - let mut file = File::open(source_tar) - .with_context(|| format!("open build context archive '{}'", source_tar.display()))?; - let mut magic = [0u8; 2]; - let gzip = match file.read(&mut magic) { - Ok(2) => magic == [0x1f, 0x8b], - _ => false, - }; - file.seek(SeekFrom::Start(0)) - .context("rewind build context archive")?; +/// The uploaded archive, opened once and read twice. +/// +/// Holding the handle across both passes means the two passes provably see the +/// same inode: a concurrent unlink or replacement of the path cannot make the +/// second pass read a different archive than the one the mapping was built +/// from. +struct SourceArchive { + file: File, + gzip: bool, + /// Hard cap on the bytes any one pass may pull out of the reader. + budget: u64, +} - let reader: Box = if gzip { - Box::new(flate2::read::GzDecoder::new(BufReader::new(file))) - } else { - Box::new(BufReader::new(file)) - }; - Ok(tar::Archive::new(reader)) +impl SourceArchive { + fn open(source_tar: &Path, max_total_bytes: u64) -> Result { + let mut file = File::open(source_tar) + .with_context(|| format!("open build context archive '{}'", source_tar.display()))?; + let mut magic = [0u8; 2]; + let gzip = match file.read(&mut magic) { + Ok(2) => magic == [0x1f, 0x8b], + _ => false, + }; + Ok(Self { + file, + gzip, + // The tar crate buffers GNU long-name and PAX records whole before + // any per-entry budget check can run, so the reader itself has to + // be capped. The slack covers per-entry framing for the maximum + // entry count on top of the caller's payload budget. + budget: max_total_bytes.saturating_add(MAX_ARCHIVE_ENTRIES as u64 * 1024), + }) + } + + /// Starts one pass over the archive from offset 0. + fn pass(&self) -> Result>> { + let mut file = self + .file + .try_clone() + .context("reopen build context archive")?; + file.seek(SeekFrom::Start(0)) + .context("rewind build context archive")?; + + let reader: Box = if self.gzip { + Box::new(flate2::read::GzDecoder::new(BufReader::new(file)).take(self.budget)) + } else { + Box::new(BufReader::new(file).take(self.budget)) + }; + Ok(tar::Archive::new(reader)) + } +} + +/// Context for a per-entry read failure. +/// +/// The reader is capped, so an entry that declares more bytes than the budget +/// allows surfaces here as a truncated-archive error rather than an unbounded +/// allocation; naming the limit keeps that case diagnosable. +fn entry_read_context(max_total_bytes: u64) -> String { + format!( + "read build context archive entry; the archive must stay within the configured \ + limit of {max_total_bytes} bytes" + ) } fn check_entry_type(entry_type: tar::EntryType) -> Result<()> { @@ -226,8 +384,8 @@ fn check_entry_type(entry_type: tar::EntryType) -> Result<()> { /// First pass: index entry paths and enforce the archive budgets without /// reading any file contents. -fn read_entry_index(source_tar: &Path, max_total_bytes: u64) -> Result> { - let mut archive = open_archive(source_tar)?; +fn read_entry_index(source: &SourceArchive, max_total_bytes: u64) -> Result> { + let mut archive = source.pass()?; let mut index = Vec::new(); let mut total_bytes = 0u64; @@ -235,11 +393,17 @@ fn read_entry_index(source_tar: &Path, max_total_bytes: u64) -> Result max_total_bytes { bail!( "build context archive expands beyond the configured limit of \ @@ -262,15 +426,27 @@ fn read_entry_index(source_tar: &Path, max_total_bytes: u64) -> Result>, + /// Resolved absolute destination root. + dest_root: String, + /// Whether a directory entry for `dest_root` itself was dropped. + skipped_dest_root: bool, + /// Whether the copy treats `dest_root` as a directory. + dest_is_dir: bool, +} + /// Computes the final absolute guest path for every indexed entry. -/// -/// The returned vector is positionally aligned with `index`. fn map_entries( index: &[EntryIndex], src: &str, dest_raw: &str, workdir: &str, -) -> Result> { +) -> Result { let src = normalize_src(src); let dest_is_dir_hint = dest_raw.ends_with('/') || dest_raw.ends_with("/.") @@ -278,8 +454,6 @@ fn map_entries( || dest_raw.is_empty(); let dest = resolve_guest_path(workdir, if dest_raw.is_empty() { "." } else { dest_raw })?; - let mut mapped = Vec::with_capacity(index.len()); - let copy_whole_context = src.is_empty() || src == "."; let single_file_src = !copy_whole_context && !is_glob_pattern(&src) @@ -287,17 +461,15 @@ fn map_entries( && index[0].path == src && !index[0].is_dir; - if single_file_src { - mapped.push(if dest_is_dir_hint { + let mapped: Vec = if single_file_src { + vec![if dest_is_dir_hint { join_abs(&dest, base_name(&src)) } else { - dest - }); - return Ok(mapped); - } - - if copy_whole_context || !is_glob_pattern(&src) { + dest.clone() + }] + } else if copy_whole_context || !is_glob_pattern(&src) { // Directory source: Docker copies the directory *contents* into dest. + let mut mapped = Vec::with_capacity(index.len()); for entry in index { let rel = if copy_whole_context { entry.path.as_str() @@ -314,54 +486,82 @@ fn map_entries( }; mapped.push(join_abs(&dest, rel)); } - return Ok(mapped); - } - - // Glob source: every matched top-level item lands inside dest. Matched - // files keep their base name; matched directories contribute their - // contents (Docker treats each matched directory like a directory source). - for entry in index { - let mut components = entry.path.split('/'); - let mut prefix = String::new(); - let mut matched_root: Option = None; - for component in components.by_ref() { - if prefix.is_empty() { - prefix.push_str(component); - } else { - prefix.push('/'); - prefix.push_str(component); - } - if glob_match(&src, &prefix) { - matched_root = Some(prefix.clone()); - break; + mapped + } else { + // Glob source: every matched top-level item lands inside dest. Matched + // files keep their base name; matched directories contribute their + // contents (Docker treats each matched directory like a directory + // source). + let mut mapped = Vec::with_capacity(index.len()); + for entry in index { + let mut components = entry.path.split('/'); + let mut prefix = String::new(); + let mut matched_root: Option = None; + for component in components.by_ref() { + if prefix.is_empty() { + prefix.push_str(component); + } else { + prefix.push('/'); + prefix.push_str(component); + } + if glob_match(&src, &prefix) { + matched_root = Some(prefix.clone()); + break; + } } + let Some(root) = matched_root else { + bail!( + "archive entry '{}' does not match COPY source pattern '{}'", + entry.path, + src + ); + }; + let rel = entry + .path + .strip_prefix(&root) + .map(|rest| rest.trim_start_matches('/')) + .unwrap_or(""); + mapped.push(if rel.is_empty() && !entry.is_dir { + join_abs(&dest, base_name(&root)) + } else { + join_abs(&dest, rel) + }); } - let Some(root) = matched_root else { - bail!( - "archive entry '{}' does not match COPY source pattern '{}'", - entry.path, - src - ); - }; - let rel = entry - .path - .strip_prefix(&root) - .map(|rest| rest.trim_start_matches('/')) - .unwrap_or(""); - mapped.push(if rel.is_empty() && !entry.is_dir { - join_abs(&dest, base_name(&root)) - } else { - join_abs(&dest, rel) - }); - } - Ok(mapped) + mapped + }; + + // A directory source (and every glob-matched directory) maps its own root + // onto dest. Emitting a header for it would make the guest's `tar -xp` + // restore mode and ownership onto a pre-existing destination directory, + // which a copy must never touch; the guest creates it instead when absent. + let mut skipped_dest_root = false; + let targets = mapped + .into_iter() + .zip(index) + .map(|(target, entry)| { + if entry.is_dir && target == dest { + skipped_dest_root = true; + None + } else { + Some(target) + } + }) + .collect(); + + Ok(MappedEntries { + targets, + dest_root: dest, + skipped_dest_root, + dest_is_dir: !single_file_src, + }) } /// Rewrites the SDK context archive into `output` with final absolute guest /// paths, the requested ownership, and the optional mode override applied. pub(crate) fn plan_copy_archive(request: &CopyRequest<'_>, output: &Path) -> Result { - let index = read_entry_index(request.source_tar, request.max_total_bytes)?; - let targets = map_entries(&index, request.src, request.dest, request.workdir)?; + let source = SourceArchive::open(request.source_tar, request.max_total_bytes)?; + let index = read_entry_index(&source, request.max_total_bytes)?; + let mapped = map_entries(&index, request.src, request.dest, request.workdir)?; let out_file = File::create(output) .with_context(|| format!("create rewritten copy archive '{}'", output.display()))?; @@ -371,16 +571,19 @@ pub(crate) fn plan_copy_archive(request: &CopyRequest<'_>, output: &Path) -> Res let mut seen = 0usize; // Second pass: stream each entry's bytes into the rewritten archive. - let mut archive = open_archive(request.source_tar)?; + let mut archive = source.pass()?; for entry in archive .entries() .context("read build context archive entries")? { - let mut entry = entry.context("read build context archive entry")?; - let Some(target) = targets.get(seen) else { + let mut entry = entry.with_context(|| entry_read_context(request.max_total_bytes))?; + let Some(target) = mapped.targets.get(seen) else { bail!("build context archive changed while it was being rewritten"); }; seen += 1; + let Some(target) = target else { + continue; + }; let relative = target.trim_start_matches('/'); if relative.is_empty() { @@ -441,7 +644,7 @@ pub(crate) fn plan_copy_archive(request: &CopyRequest<'_>, output: &Path) -> Res entry_count += 1; } - if seen != targets.len() { + if seen != mapped.targets.len() { bail!("build context archive changed while it was being rewritten"); } @@ -451,6 +654,9 @@ pub(crate) fn plan_copy_archive(request: &CopyRequest<'_>, output: &Path) -> Res Ok(CopyPlan { entry_count, total_bytes, + dest_root: mapped.dest_root, + skipped_dest_root: mapped.skipped_dest_root, + dest_is_dir: mapped.dest_is_dir, }) } @@ -556,6 +762,10 @@ mod tests { assert_eq!(plan.entry_count, 1); assert_eq!(plan.total_bytes, 6); + assert!( + !plan.dest_is_dir, + "a single-file dest needs no directory preparation" + ); let entries = rewritten_entries(&out); let entry = &entries["hello.txt"]; assert_eq!(entry.kind, tar::EntryType::Regular); @@ -601,10 +811,20 @@ mod tests { ); let out = dir.path().join("out.tar"); - plan_copy_archive(&request(&tar, "app", "/opt/service", "/"), &out).expect("plan"); + let plan = + plan_copy_archive(&request(&tar, "app", "/opt/service", "/"), &out).expect("plan"); + assert_eq!(plan.dest_root, "/opt/service"); + assert!( + plan.skipped_dest_root, + "the destination root must be left to the guest" + ); + assert!(plan.dest_is_dir); let entries = rewritten_entries(&out); - assert!(entries.contains_key("opt/service/")); + assert!( + !entries.contains_key("opt/service/"), + "a header for the destination root would rewrite its metadata" + ); assert!(entries.contains_key("opt/service/main.py")); assert!(entries.contains_key("opt/service/sub/")); assert!(entries.contains_key("opt/service/sub/util.py")); @@ -661,11 +881,16 @@ mod tests { ); let out = dir.path().join("out.tar"); - plan_copy_archive(&request(&tar, "pkg-*", "/opt/pkgs", "/"), &out).expect("plan"); + let plan = + plan_copy_archive(&request(&tar, "pkg-*", "/opt/pkgs", "/"), &out).expect("plan"); // Docker merges contents of every matched directory into dest; the // second lib.py overwrites the first at extract time. - assert!(rewritten_entries(&out).contains_key("opt/pkgs/lib.py")); + let entries = rewritten_entries(&out); + assert!(entries.contains_key("opt/pkgs/lib.py")); + assert_eq!(plan.dest_root, "/opt/pkgs"); + assert!(plan.skipped_dest_root); + assert!(!entries.contains_key("opt/pkgs/")); } #[test] @@ -735,6 +960,58 @@ mod tests { assert!(err.to_string().contains("expands beyond")); } + #[test] + fn rejects_truncated_long_name_records() { + let dir = TempDir::new().expect("tempdir"); + let inner = build_source_tar(dir.path(), &[("small.txt", Some("s"))]); + + // A GNU long-name record declaring far more bytes than it carries. + // The tar crate buffers such a record whole; the capped reader bounds + // that allocation and the overdeclared record fails as truncated + // instead of being served the rest of the stream as name bytes. + let mut header = tar::Header::new_gnu(); + let long_link = b"././@LongLink"; + header.as_gnu_mut().expect("gnu header").name[..long_link.len()].copy_from_slice(long_link); + header.set_entry_type(tar::EntryType::GNULongName); + header.set_mode(0o644); + header.set_size(0o77777777777); + header.set_cksum(); + + let mut bytes = Vec::new(); + bytes.extend_from_slice(header.as_bytes()); + bytes.extend_from_slice(&[0u8; 512]); + bytes.extend_from_slice(&std::fs::read(&inner).expect("read inner tar")); + let tar_path = dir.path().join("longname.tar"); + std::fs::write(&tar_path, &bytes).expect("write tar"); + let out = dir.path().join("out.tar"); + + let mut req = request(&tar_path, "small.txt", "/small.txt", "/"); + req.max_total_bytes = 64 * 1024; + let err = plan_copy_archive(&req, &out).expect_err("overdeclared long-name must fail"); + assert!(err.to_string().contains("configured limit")); + } + + #[test] + fn counts_entry_framing_against_the_budget() { + let dir = TempDir::new().expect("tempdir"); + // Twenty empty files carry zero payload bytes but 512 bytes of tar + // framing each, which the index pass must charge to the budget. + let entries: Vec<(String, Option<&str>)> = (0..20) + .map(|i| (format!("empty-{i}.txt"), Some(""))) + .collect(); + let entries: Vec<(&str, Option<&str>)> = entries + .iter() + .map(|(name, content)| (name.as_str(), *content)) + .collect(); + let tar = build_source_tar(dir.path(), &entries); + let out = dir.path().join("out.tar"); + + let mut req = request(&tar, ".", "/ctx/", "/"); + req.max_total_bytes = 4 * 1024; + let err = plan_copy_archive(&req, &out).expect_err("framing must exhaust the budget"); + assert!(err.to_string().contains("expands beyond")); + } + #[test] fn rejects_entries_escaping_the_root() { let dir = TempDir::new().expect("tempdir"); @@ -761,6 +1038,32 @@ mod tests { assert!(err.to_string().contains("unsupported path component")); } + #[test] + fn rejects_non_utf8_entry_names() { + let dir = TempDir::new().expect("tempdir"); + let tar_path = dir.path().join("latin1.tar"); + let file = File::create(&tar_path).expect("create tar"); + let mut builder = tar::Builder::new(file); + let mut header = tar::Header::new_gnu(); + // A latin-1 name; lossy conversion would silently rename it and + // collapse distinct names onto one replacement-character path. + let raw_name = b"caf\xe9.txt"; + header.as_gnu_mut().expect("gnu header").name[..raw_name.len()].copy_from_slice(raw_name); + header.set_entry_type(tar::EntryType::Regular); + header.set_mode(0o644); + header.set_size(1); + header.set_cksum(); + builder + .append(&header, "x".as_bytes()) + .expect("append raw entry"); + builder.finish().expect("finish"); + let out = dir.path().join("out.tar"); + + let err = plan_copy_archive(&request(&tar_path, ".", "/ctx/", "/"), &out) + .expect_err("non-UTF-8 entry name must fail"); + assert!(err.to_string().contains("non-UTF-8 path component")); + } + #[test] fn rejects_dest_escaping_the_root() { let dir = TempDir::new().expect("tempdir"); @@ -813,4 +1116,24 @@ mod tests { assert!(!glob_match("[!ab]*", "b12")); assert!(glob_match("pkg-*", "pkg-a")); } + + #[test] + fn glob_wildcards_never_cross_a_separator() { + assert!(!glob_match("*.txt", "sub/a.txt")); + assert!(!glob_match("src?nested", "src/nested")); + assert!(!glob_match("[sa]rc", "src/nested")); + assert!(glob_match("src/*.rs", "src/main.rs")); + assert!(!glob_match("src/*.rs", "src/nested/main.rs")); + assert!(!glob_match("src/*", "src")); + } + + #[test] + fn glob_match_stays_polynomial_on_pathological_patterns() { + // The previous recursive matcher took minutes on this input. + assert!(!glob_match("*a*a*a*a*a*a*a*a*b", &"a".repeat(64))); + assert!(glob_match( + "*a*a*a*a*a*a*a*a*b", + &format!("{}b", "a".repeat(64)) + )); + } } diff --git a/src/template/step_executor.rs b/src/template/step_executor.rs index f3b2adc3..79945c53 100644 --- a/src/template/step_executor.rs +++ b/src/template/step_executor.rs @@ -18,7 +18,9 @@ use crate::snapshot::CommandContext; /// this stays conservative rather than mirroring every libc name rule. fn is_valid_chown_spec(spec: &str) -> bool { fn valid_part(part: &str) -> bool { + // A leading '-' would be read as an option by the `id` lookup below. !part.is_empty() + && !part.starts_with('-') && part .bytes() .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'_' | b'-' | b'.')) @@ -73,6 +75,35 @@ impl TemplateStepExecutor { format!("WORKDIR {path}"), ) })?; + // Docker creates the directory. A bare `mkdir -p` matches + // the classic builder: idempotent on an existing directory + // without touching its metadata, and ENOTDIR on a file. + // No cwd, because the current workdir may not exist. + let script = format!("set -eu\nmkdir -p -- {}\n", shell_quote(&resolved)); + let output = sandbox + .run_command_with_opts( + "/bin/bash", + &["-lc", &script], + &ProcessOpts::default(), + ) + .await + .with_context(|| { + TemplateBuildFailure::with_step( + "build step failed: create workdir", + format!("WORKDIR {path}"), + ) + })?; + if output.exit_code != 0 { + return Err(TemplateBuildFailure::with_step( + format!( + "build step failed: creating the workdir exited with status {}{}", + output.exit_code, + command_output_suffix(&output.stdout, &output.stderr) + ), + format!("WORKDIR {path}"), + ) + .into()); + } context = context.with_workdir(resolved); } TemplateBuildStepKind::User { value } => { @@ -202,10 +233,35 @@ impl TemplateStepExecutor { .await .with_context(|| with_step("build step failed: upload build context".to_string()))?; - let script = format!( - "tar -xpf {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", + // The plan never emits a header for the destination root, so a + // pre-existing destination keeps its metadata. Create it here when it + // is missing so a fresh destination still gets the requested metadata; + // `tar -C /` auto-creates any missing intermediate directory. File-only + // archives carry no directory member for the root, hence the + // `dest_is_dir` gate rather than `skipped_dest_root` alone. + let mut script = String::new(); + if plan.skipped_dest_root || plan.dest_is_dir { + let dest_root = shell_quote(&plan.dest_root); + script.push_str(&format!("if [ ! -e {dest_root} ]; then\n")); + script.push_str(&format!(" mkdir -p -- {dest_root} || exit 1\n")); + if let Some(owner) = ownership { + script.push_str(&format!( + " chown {}:{} -- {dest_root} || exit 1\n", + owner.uid, owner.gid + )); + } + if let Some(mode) = mode { + script.push_str(&format!(" chmod {mode:o} -- {dest_root} || exit 1\n")); + } + script.push_str("fi\n"); + } + // `--no-overwrite-dir` (GNU tar) keeps extraction from restoring mode + // and ownership onto directories that already exist in the image; + // directories tar creates still receive the archive header's metadata. + script.push_str(&format!( + "tar -xp --no-overwrite-dir -f {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", archive = shell_quote(&guest_archive), - ); + )); let output = sandbox .run_command_with_opts("/bin/bash", &["-lc", &script], &ProcessOpts::default()) @@ -238,7 +294,7 @@ impl TemplateStepExecutor { let script = format!( r#"set -eu case "{user}" in - *[!0-9]*) uid=$(id -u "{user}"); ugid=$(id -g "{user}") ;; + *[!0-9]*) uid=$(id -u -- "{user}"); ugid=$(id -g -- "{user}") ;; *) uid="{user}"; ugid="{user}" ;; esac if [ -n "{group}" ]; then @@ -329,15 +385,19 @@ printf '%s %s\n' "$uid" "$gid" #[cfg(test)] mod tests { use std::collections::HashMap; + use std::sync::Mutex; use anyhow::{anyhow, Result}; use async_trait::async_trait; + use shell_util::shell_quote; use super::TemplateStepExecutor; use crate::sandbox::{Executor, ProcessHandle, ProcessOpts, ProcessOutput, SandboxExecutor}; use crate::snapshot::CommandContext; use crate::template::build_spec::TemplateBuildStep; + /// Sandbox that fails any exec, so steps expected to stay host-side prove + /// they never touch the guest. struct NoopSandbox; #[async_trait(?Send)] @@ -363,6 +423,59 @@ mod tests { } } + /// One recorded exec: command, arguments, and working directory. + type RecordedCommand = (String, Vec, Option); + + /// Records every command a step issues and reports success. + #[derive(Default)] + struct RecordingSandbox { + commands: Mutex>, + } + + impl RecordingSandbox { + fn commands(&self) -> Vec { + self.commands + .lock() + .expect("commands mutex should not be poisoned") + .clone() + } + } + + #[async_trait(?Send)] + impl SandboxExecutor for RecordingSandbox { + fn executor(&self) -> Result> { + Err(anyhow!("not used by this test")) + } + async fn run_command_with_opts( + &self, + cmd: &str, + args: &[&str], + opts: &ProcessOpts, + ) -> Result { + self.commands + .lock() + .expect("commands mutex should not be poisoned") + .push(( + cmd.to_string(), + args.iter().map(|arg| (*arg).to_string()).collect(), + opts.cwd.clone(), + )); + Ok(ProcessOutput { + stdout: String::new(), + stderr: String::new(), + exit_code: 0, + }) + } + async fn start_process( + &self, + _cmd: &str, + _args: &[&str], + _opts: &ProcessOpts, + ) -> Result { + Err(anyhow!("not used by this test")) + } + } + async fn run(steps: Vec) -> CommandContext { TemplateStepExecutor::new() .execute( @@ -453,6 +566,8 @@ mod tests { assert!(!super::is_valid_chown_spec("user:group:extra")); assert!(!super::is_valid_chown_spec("user name")); assert!(!super::is_valid_chown_spec("user;rm -rf /")); + assert!(!super::is_valid_chown_spec("-r")); + assert!(!super::is_valid_chown_spec("user:-g")); } #[tokio::test] @@ -474,7 +589,50 @@ mod tests { #[tokio::test] async fn workdir_step_updates_workdir() { - let ctx = run(vec![TemplateBuildStep::workdir("/workspace")]).await; + let sandbox = RecordingSandbox::default(); + let ctx = TemplateStepExecutor::new() + .execute( + &sandbox, + &[TemplateBuildStep::workdir("/workspace")], + CommandContext::default(), + &HashMap::new(), + ) + .await + .expect("steps should execute without error"); + assert_eq!(ctx.workdir, "/workspace"); + let commands = sandbox.commands(); + assert_eq!(commands.len(), 1); + assert!( + commands[0].1[1].contains(&shell_quote("/workspace")), + "WORKDIR must create the directory: {}", + commands[0].1[1] + ); + } + + #[tokio::test] + async fn workdir_step_resolves_relative_paths() { + let sandbox = RecordingSandbox::default(); + let ctx = TemplateStepExecutor::new() + .execute( + &sandbox, + &[ + TemplateBuildStep::workdir("/a"), + TemplateBuildStep::workdir("b"), + ], + CommandContext::default(), + &HashMap::new(), + ) + .await + .expect("steps should execute without error"); + + assert_eq!(ctx.workdir, "/a/b"); + let commands = sandbox.commands(); + assert_eq!(commands.len(), 2); + assert!( + commands[1].1[1].contains(&shell_quote("/a/b")), + "the second WORKDIR must create the resolved path: {}", + commands[1].1[1] + ); } } From 6f4b6a5b0d51855fe90aaa645849db92099e4963 Mon Sep 17 00:00:00 2001 From: JoyboyBrian Date: Tue, 28 Jul 2026 12:06:53 -0700 Subject: [PATCH 7/7] fix(template): address round-2 review findings Upload lifecycle: the handler now runs verify -> stage -> import -> claim, so a failed store leaves the single-use URL retryable; the grant is consumed only after the archive is durably published (safe: the token binds one template_id/hash and import is first-write-wins). posixfs fsyncs the staged archive before the hard-link publish, caps opportunistic pruning at 256 entries per call, and catalog create is all-or-nothing under the alias lock. copy_plan: the capped-reader slack scales with the configured budget instead of a fixed ~195 MiB; a glob resolving to exactly one regular file gets single-file destination semantics with the base name taken from the resolved entry; an explicit "dest/" directory destination is prepared for single-file copies without inheriting the file's --chmod mode. Validation and errors: filesHash is shape-checked at the API edge with the store's own validator; files_max_upload_mib and files_max_context_mib reject zero at startup; envd upload errors keep their source chain; a failed COPY ownership lookup no longer asserts the user does not exist. --- src/api/build_files.rs | 41 +++--- src/api/impls/template_helpers.rs | 22 +++ src/cfg.rs | 24 ++++ src/sandbox/envd.rs | 2 +- .../backends/posixfs/build_files.rs | 39 +++++- .../repository/backends/posixfs/catalog.rs | 40 ++++-- src/snapshot/repository/build_files.rs | 9 +- src/template/copy_plan.rs | 71 ++++++++-- src/template/step_executor.rs | 132 ++++++++++++++++-- 9 files changed, 321 insertions(+), 59 deletions(-) diff --git a/src/api/build_files.rs b/src/api/build_files.rs index 28d80d43..d911fbe5 100644 --- a/src/api/build_files.rs +++ b/src/api/build_files.rs @@ -94,8 +94,9 @@ where ); }; - // Verification does not consume the grant, so an upload that fails before - // the archive is stored can be retried with the same URL. + // Verification does not consume the grant: consumption happens only after + // the archive has been durably published, so an upload that fails while + // streaming, staging, or storing the body stays retryable with this URL. let now_unix = chrono::Utc::now().timestamp(); let authorized = match store .verify_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) @@ -214,10 +215,28 @@ where }; drop(file); - // Consuming the grant here keeps a failed upload retryable while the - // atomic remove/delete still picks a single winner among concurrent - // replays. `now_unix` is the timestamp taken before the body was read, so - // a slow but authorized upload is not rejected for aging past the TTL. + // Publishing before the grant is consumed keeps a failed store retryable + // with the same URL. An unclaimed replay reaching this point is harmless: + // the token authorizes exactly this template_id/hash and `import` is + // first-write-wins, so it can neither publish a different key nor change + // what is already stored. + // + // `hash` is the cache key the SDK computed for this build context, not a + // digest of the received bytes that the server verified. + if let Err(error) = store.import(&hash, &staged_path).await { + warn!(error = %error, hash, "failed to import build archive"); + return error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to store build archive; the upload can be retried with the same link", + ); + } + + // The archive is published, so the claim only enforces single-use: the + // atomic remove/delete picks a single winner among concurrent replays, and + // a replay that loses the race is rejected even though the archive it + // uploaded is stored. `now_unix` is the timestamp taken before the body was + // read, so a slow but authorized upload is not rejected for aging past the + // TTL. let claimed = match store .claim_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) .await @@ -238,16 +257,6 @@ where ); } - // `hash` is the cache key the SDK computed for this build context, not a - // digest of the received bytes that the server verified. - if let Err(error) = store.import(&hash, &staged_path).await { - warn!(error = %error, hash, "failed to import build archive"); - return error_response( - StatusCode::INTERNAL_SERVER_ERROR, - "failed to store build archive", - ); - } - debug!( template_id, hash, diff --git a/src/api/impls/template_helpers.rs b/src/api/impls/template_helpers.rs index 3bd4c3b0..d4a61ca6 100644 --- a/src/api/impls/template_helpers.rs +++ b/src/api/impls/template_helpers.rs @@ -1,6 +1,7 @@ use agentenv_http_server::models; use crate::cfg::ConfigManager; +use crate::snapshot::repository::build_files::is_valid_build_files_hash; use crate::snapshot::{SnapshotAlias, SnapshotId, SnapshotRecord}; use crate::template::TemplateBuildSpec; use crate::types::SandboxResources; @@ -248,6 +249,17 @@ fn apply_e2b_template_step( ), )); }; + // The hash is an opaque cache key that becomes part of an archive + // path, so it must be shape-checked before the build starts. + if !is_valid_build_files_hash(files_hash) { + return Err(models::Error::new( + 400, + format!( + "{} template step filesHash '{files_hash}' is not a valid build files hash", + step.r_type + ), + )); + } let src = args .first() .map(|value| value.trim()) @@ -416,6 +428,16 @@ mod tests { assert!(err.message.contains("10000"), "{}", err.message); } + #[test] + fn copy_step_rejects_a_malformed_files_hash() { + let mut copy = copy_step(&["src", "/dest"]); + copy.files_hash = Some("../../etc/passwd".to_string()); + + let err = apply(©).expect_err("a malformed filesHash should be rejected"); + assert_eq!(err.code, 400); + assert!(err.message.contains("../../etc/passwd"), "{}", err.message); + } + #[test] fn copy_step_rejects_an_oversized_source_pattern() { let oversized = "a".repeat(MAX_COPY_SRC_BYTES + 1); diff --git a/src/cfg.rs b/src/cfg.rs index d525f25a..533b107f 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -863,6 +863,12 @@ impl AppConfig { cfg.files_url_ttl_secs ); } + if cfg.files_max_upload_mib == 0 { + bail!("template_build.files_max_upload_mib must be > 0"); + } + if cfg.files_max_context_mib == 0 { + bail!("template_build.files_max_context_mib must be > 0"); + } if cfg.files_max_build_context_mib == 0 { bail!("template_build.files_max_build_context_mib must be > 0"); } @@ -1360,6 +1366,24 @@ mod tests { "unexpected error: {err}" ); + let mut config = AppConfig::default(); + config.template_build.files_max_upload_mib = 0; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("template_build.files_max_upload_mib must be > 0"), + "unexpected error: {err}" + ); + + let mut config = AppConfig::default(); + config.template_build.files_max_context_mib = 0; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("template_build.files_max_context_mib must be > 0"), + "unexpected error: {err}" + ); + let mut config = AppConfig::default(); config.template_build.files_max_build_context_mib = 0; let err = config.validate().unwrap_err(); diff --git a/src/sandbox/envd.rs b/src/sandbox/envd.rs index 074c6a0a..b33209cd 100644 --- a/src/sandbox/envd.rs +++ b/src/sandbox/envd.rs @@ -110,7 +110,7 @@ impl EnvdInstance { debug!(%error, "ignoring envd upload response-body decoding error"); Ok(()) } - Err(error) => Err(anyhow!("upload file to sandbox via envd: {error}")), + Err(error) => Err(anyhow::Error::new(error).context("upload file to sandbox via envd")), } } diff --git a/src/snapshot/repository/backends/posixfs/build_files.rs b/src/snapshot/repository/backends/posixfs/build_files.rs index 4005ff12..c486ee8b 100644 --- a/src/snapshot/repository/backends/posixfs/build_files.rs +++ b/src/snapshot/repository/backends/posixfs/build_files.rs @@ -58,7 +58,8 @@ impl PosixFsTemplateBuildFileStore { } /// Removes archives whose modification time is older than the retention - /// window. Runs opportunistically on import; failures only log. + /// window. Runs opportunistically on import and scans a bounded number of + /// entries per call; failures only log. fn prune_expired(root: &Path) { let cutoff = SystemTime::now() - BUILD_FILE_RETENTION; Self::prune_dir_older_than(root, "tar", cutoff); @@ -66,7 +67,9 @@ impl PosixFsTemplateBuildFileStore { /// Removes upload grants that have passed their own `expires_unix`. Runs /// opportunistically whenever a new grant is written, so the grants - /// directory stays bounded by upload-link traffic; failures only log. + /// directory stays bounded by upload-link traffic; the scan is bounded per + /// call and drains the backlog over successive requests, and failures only + /// log. /// /// Pruning by the record rather than by mtime keeps grants alive for /// exactly their TTL even when `template_build.files_url_ttl_secs` is @@ -92,19 +95,30 @@ impl PosixFsTemplateBuildFileStore { }); } + /// Pruning is opportunistic and bounded: at most `MAX_PRUNE_SCAN` matching + /// entries are inspected per call, so the cost a request pays stays + /// constant no matter how many records the directory holds. Anything left + /// over is reclaimed by later calls. fn prune_dir( dir: &Path, extension: &str, is_expired: impl Fn(&Path, Option) -> bool, ) { + const MAX_PRUNE_SCAN: usize = 256; + let Ok(entries) = fs::read_dir(dir) else { return; }; + let mut scanned: usize = 0; for entry in entries.flatten() { let path = entry.path(); if path.extension().is_none_or(|ext| ext != extension) { continue; } + if scanned >= MAX_PRUNE_SCAN { + break; + } + scanned += 1; let modified = entry .metadata() .and_then(|metadata| metadata.modified()) @@ -248,6 +262,16 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { let _ = fs::remove_file(&store_staged); RepositoryError::backend("copy build archive into store", error) })?; + // The archive is only ever published once, so its data must reach + // stable storage before the name does: a directory entry that + // outlives the bytes would pin a truncated archive forever behind + // the `exists` fast path. + fs::File::open(&store_staged) + .and_then(|file| file.sync_all()) + .map_err(|error| { + let _ = fs::remove_file(&store_staged); + RepositoryError::backend("sync build archive", error) + })?; // Link rather than rename so a concurrent import cannot replace an // archive a running build is already reading: the first writer // wins and everyone else observes `AlreadyExists`. @@ -256,6 +280,17 @@ impl TemplateBuildFileStore for PosixFsTemplateBuildFileStore { Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(()), Err(error) => Err(RepositoryError::backend("publish build archive", error)), }; + if published.is_ok() { + // Best effort: filesystems that reject a directory fsync must + // keep working, and a lost entry only costs one re-upload. + if let Err(error) = fs::File::open(&root).and_then(|dir| dir.sync_all()) { + debug!( + path = %root.display(), + error = %error, + "failed to sync build archive store directory" + ); + } + } let _ = fs::remove_file(&store_staged); published }) diff --git a/src/snapshot/repository/backends/posixfs/catalog.rs b/src/snapshot/repository/backends/posixfs/catalog.rs index e617fdc2..76b7486c 100644 --- a/src/snapshot/repository/backends/posixfs/catalog.rs +++ b/src/snapshot/repository/backends/posixfs/catalog.rs @@ -162,23 +162,33 @@ impl PosixFsCatalogStore { self.with_alias_lock(alias, |store| { store.write_record_unlocked(&record)?; let alias_path = PosixFsSnapshotArtifactLayout::alias_path(&store.root, alias); - match store.load_alias_target(alias)? { - // The alias currently points at a live snapshot. Leave the - // binding untouched so the existing template keeps resolving - // while the new build runs; a successful commit moves the - // alias to the new snapshot (E2B rebuild semantics). - Some(existing) - if existing != record.id - && store.load_record_by_id_unlocked(&existing)?.is_some() => - { - Ok(()) - } - Some(existing) if existing != record.id => { - store.remove_file_if_exists(&alias_path)?; - store.write_json(&alias_path, &record.id) + let bind = (|| -> RepositoryResult<()> { + match store.load_alias_target(alias)? { + // The alias currently points at a live snapshot. Leave the + // binding untouched so the existing template keeps resolving + // while the new build runs; a successful commit moves the + // alias to the new snapshot (E2B rebuild semantics). + Some(existing) + if existing != record.id + && store.load_record_by_id_unlocked(&existing)?.is_some() => + { + Ok(()) + } + Some(existing) if existing != record.id => { + store.remove_file_if_exists(&alias_path)?; + store.write_json(&alias_path, &record.id) + } + _ => store.write_json(&alias_path, &record.id), } - _ => store.write_json(&alias_path, &record.id), + })(); + if let Err(error) = bind { + // Keep creation all-or-nothing under the alias lock: a record + // that survives a failed binding claims the alias in listings + // with nothing left to reconcile it. + let _ = store.remove_file_if_exists(&store.record_path(&record.id)); + return Err(error); } + Ok(()) })?; } else { self.write_record_unlocked(&record)?; diff --git a/src/snapshot/repository/build_files.rs b/src/snapshot/repository/build_files.rs index 163442a9..f693af92 100644 --- a/src/snapshot/repository/build_files.rs +++ b/src/snapshot/repository/build_files.rs @@ -100,7 +100,8 @@ pub trait TemplateBuildFileStore: Send + Sync { /// /// Verification never removes the grant, so a request that fails before /// the archive is stored can be retried with the same upload URL. Callers - /// must still `claim_upload_grant` before publishing the archive. + /// must `claim_upload_grant` after publishing the archive, so a failed + /// publication leaves the URL retryable. async fn verify_upload_grant( &self, token: &str, @@ -120,8 +121,10 @@ pub trait TemplateBuildFileStore: Send + Sync { /// concurrent requests carrying the same token cannot both succeed. /// S3-compatible backends have no conditional delete and therefore /// degrade to best-effort single-use within the grant TTL; archive - /// immutability is what keeps a lost race from mattering, since the - /// competing uploads can only publish the same content-addressed archive. + /// immutability is what keeps a lost race from mattering: both uploads are + /// bound to the same (template_id, hash), and `import` is first-write-wins, + /// so neither can change an archive that is already stored — which upload + /// wins a first store is undefined. async fn claim_upload_grant( &self, token: &str, diff --git a/src/template/copy_plan.rs b/src/template/copy_plan.rs index 27feae23..f5302a9f 100644 --- a/src/template/copy_plan.rs +++ b/src/template/copy_plan.rs @@ -329,14 +329,22 @@ impl SourceArchive { Ok(2) => magic == [0x1f, 0x8b], _ => false, }; + // Every indexed entry charges at least its own 512-byte header against + // the payload budget, so the configured limit already bounds how many + // entries an archive can hold. + let max_entries = (max_total_bytes / 512 + 1).min(MAX_ARCHIVE_ENTRIES as u64); Ok(Self { file, gzip, // The tar crate buffers GNU long-name and PAX records whole before // any per-entry budget check can run, so the reader itself has to - // be capped. The slack covers per-entry framing for the maximum - // entry count on top of the caller's payload budget. - budget: max_total_bytes.saturating_add(MAX_ARCHIVE_ENTRIES as u64 * 1024), + // be capped. The slack allows 1 KiB of framing for every entry the + // budget can hold plus the end-of-archive terminator, which keeps + // long-name-heavy archives acceptable while keeping the raw stream + // proportional to the caller's payload budget at any setting. + budget: max_total_bytes + .saturating_add(max_entries.saturating_mul(1024)) + .saturating_add(1024), }) } @@ -455,15 +463,20 @@ fn map_entries( let dest = resolve_guest_path(workdir, if dest_raw.is_empty() { "." } else { dest_raw })?; let copy_whole_context = src.is_empty() || src == "."; + // Docker gives a wildcard that resolves to exactly one regular file the + // same destination semantics as a literal single-file source. A matched + // directory always contributes its own member, so `!is_dir` is what keeps + // directory sources on the recursive path. let single_file_src = !copy_whole_context - && !is_glob_pattern(&src) && index.len() == 1 - && index[0].path == src - && !index[0].is_dir; + && !index[0].is_dir + && (index[0].path == src || glob_match(&src, &index[0].path)); let mapped: Vec = if single_file_src { vec![if dest_is_dir_hint { - join_abs(&dest, base_name(&src)) + // The base name has to come from the resolved entry: the source + // may be a pattern, which is never a valid path component. + join_abs(&dest, base_name(&index[0].path)) } else { dest.clone() }] @@ -552,7 +565,10 @@ fn map_entries( targets, dest_root: dest, skipped_dest_root, - dest_is_dir: !single_file_src, + // An explicit directory destination ("dest/") is a directory even for + // a single-file copy, and the guest still has to create it when it is + // missing: no archive member covers the destination root itself. + dest_is_dir: !single_file_src || dest_is_dir_hint, }) } @@ -780,9 +796,14 @@ mod tests { let tar = build_source_tar(dir.path(), &[("requirements.txt", Some("e2b\n"))]); let out = dir.path().join("out.tar"); - plan_copy_archive(&request(&tar, "requirements.txt", "/home/user/", "/"), &out) + let plan = plan_copy_archive(&request(&tar, "requirements.txt", "/home/user/", "/"), &out) .expect("plan"); + assert_eq!(plan.dest_root, "/home/user"); + assert!( + plan.dest_is_dir, + "an explicit directory destination must be prepared by the guest" + ); assert!(rewritten_entries(&out).contains_key("home/user/requirements.txt")); } @@ -867,6 +888,34 @@ mod tests { assert!(entries.contains_key("data/two.txt")); } + #[test] + fn glob_matching_one_file_renames_onto_a_file_dest() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("one.txt", Some("1"))]); + let out = dir.path().join("out.tar"); + + let plan = + plan_copy_archive(&request(&tar, "*.txt", "/renamed.txt", "/"), &out).expect("plan"); + + assert!( + !plan.dest_is_dir, + "a wildcard resolving to one file renames like a literal source" + ); + assert!(rewritten_entries(&out).contains_key("renamed.txt")); + } + + #[test] + fn glob_matching_one_file_keeps_its_name_under_a_directory_dest() { + let dir = TempDir::new().expect("tempdir"); + let tar = build_source_tar(dir.path(), &[("one.txt", Some("1"))]); + let out = dir.path().join("out.tar"); + + plan_copy_archive(&request(&tar, "*.txt", "/data/", "/"), &out).expect("plan"); + + // The base name comes from the matched entry, never from the pattern. + assert!(rewritten_entries(&out).contains_key("data/one.txt")); + } + #[test] fn glob_matching_directory_copies_its_contents() { let dir = TempDir::new().expect("tempdir"); @@ -968,7 +1017,9 @@ mod tests { // A GNU long-name record declaring far more bytes than it carries. // The tar crate buffers such a record whole; the capped reader bounds // that allocation and the overdeclared record fails as truncated - // instead of being served the rest of the stream as name bytes. + // instead of being served the rest of the stream as name bytes. The + // cap scales with the configured budget, so the 64 KiB limit below + // bounds the allocation at KiB rather than MiB scale. let mut header = tar::Header::new_gnu(); let long_link = b"././@LongLink"; header.as_gnu_mut().expect("gnu header").name[..long_link.len()].copy_from_slice(long_link); diff --git a/src/template/step_executor.rs b/src/template/step_executor.rs index 79945c53..df91e885 100644 --- a/src/template/step_executor.rs +++ b/src/template/step_executor.rs @@ -235,10 +235,11 @@ impl TemplateStepExecutor { // The plan never emits a header for the destination root, so a // pre-existing destination keeps its metadata. Create it here when it - // is missing so a fresh destination still gets the requested metadata; - // `tar -C /` auto-creates any missing intermediate directory. File-only - // archives carry no directory member for the root, hence the - // `dest_is_dir` gate rather than `skipped_dest_root` alone. + // is missing so a fresh destination still gets the requested + // ownership; `tar -C /` auto-creates any missing intermediate + // directory. File-only archives carry no directory member for the + // root, hence the `dest_is_dir` gate rather than `skipped_dest_root` + // alone. let mut script = String::new(); if plan.skipped_dest_root || plan.dest_is_dir { let dest_root = shell_quote(&plan.dest_root); @@ -250,7 +251,12 @@ impl TemplateStepExecutor { owner.uid, owner.gid )); } - if let Some(mode) = mode { + // `--chmod` applies to copied content only. `skipped_dest_root` + // means the archive itself carried the destination directory, so + // the mode is the one that entry would have received; a directory + // synthesized for a single-file copy is not copied content and + // must not take the file's mode. + if let Some(mode) = mode.filter(|_| plan.skipped_dest_root) { script.push_str(&format!(" chmod {mode:o} -- {dest_root} || exit 1\n")); } script.push_str("fi\n"); @@ -281,9 +287,9 @@ impl TemplateStepExecutor { /// Resolves a `--chown` spec to numeric ids inside the build sandbox. /// /// Docker resolves names against the image's own `/etc/passwd` and - /// `/etc/group`, so the lookup has to happen in the guest. Failing here - /// reports an unknown user the same way Docker does rather than silently - /// falling back to root. + /// `/etc/group`, so the lookup has to happen in the guest. A failed lookup + /// fails the step the way Docker does rather than silently falling back to + /// root. async fn resolve_ownership( &self, sandbox: &impl SandboxExecutor, @@ -321,9 +327,13 @@ printf '%s %s\n' "$uid" "$gid" ) })?; if output.exit_code != 0 { + // A nonzero exit also covers a missing group, a `/etc/group` the + // lookup cannot read, and a guest without `id`/`awk`, so the + // message must not assert that the user is absent. return Err(TemplateBuildFailure::with_step( format!( - "build step failed: COPY user '{user}' does not exist in the image{}", + "build step failed: could not resolve COPY ownership '{user}' in the image; \ + the user or group may not exist{}", command_output_suffix(&output.stdout, &output.stderr) ), step_label, @@ -426,10 +436,14 @@ mod tests { /// One recorded exec: command, arguments, and working directory. type RecordedCommand = (String, Vec, Option); - /// Records every command a step issues and reports success. + /// Records every command a step issues and replays a canned result. #[derive(Default)] struct RecordingSandbox { commands: Mutex>, + /// Stdout every recorded command reports back. + stdout: String, + /// Exit code every recorded command reports back. + exit_code: i32, } impl RecordingSandbox { @@ -461,9 +475,9 @@ mod tests { opts.cwd.clone(), )); Ok(ProcessOutput { - stdout: String::new(), + stdout: self.stdout.clone(), stderr: String::new(), - exit_code: 0, + exit_code: self.exit_code, }) } async fn start_process( @@ -474,6 +488,31 @@ mod tests { ) -> Result { Err(anyhow!("not used by this test")) } + async fn upload_file( + &self, + _local_path: &std::path::Path, + _guest_path: &str, + _username: &str, + ) -> Result<()> { + Ok(()) + } + } + + /// Writes a one-file build context archive and returns its path. + fn single_file_archive(dir: &std::path::Path) -> std::path::PathBuf { + let tar_path = dir.join("context.tar"); + let mut builder = + tar::Builder::new(std::fs::File::create(&tar_path).expect("create context tar")); + let contents = b"e2b\n"; + let mut header = tar::Header::new_gnu(); + header.set_entry_type(tar::EntryType::Regular); + header.set_mode(0o644); + header.set_size(contents.len() as u64); + builder + .append_data(&mut header, "requirements.txt", &contents[..]) + .expect("append file"); + builder.finish().expect("finish context tar"); + tar_path } async fn run(steps: Vec) -> CommandContext { @@ -555,6 +594,75 @@ mod tests { assert!(err.to_string().contains("has not been uploaded")); } + #[tokio::test] + async fn copy_step_prepares_a_directory_dest_without_chmod() { + let dir = tempfile::TempDir::new().expect("tempdir"); + let mut archives = HashMap::new(); + archives.insert( + "aabbccddeeff0011".to_string(), + single_file_archive(dir.path()), + ); + let sandbox = RecordingSandbox { + stdout: "1000 2000\n".to_string(), + ..RecordingSandbox::default() + }; + + TemplateStepExecutor::new() + .execute( + &sandbox, + &[TemplateBuildStep::copy( + "requirements.txt", + "/home/user/", + "aabbccddeeff0011", + Some("1000:2000".to_string()), + Some(0o600), + )], + CommandContext::default(), + &archives, + ) + .await + .expect("copy step should execute"); + + let commands = sandbox.commands(); + let script = &commands.last().expect("extraction command").1[1]; + let dest_root = shell_quote("/home/user"); + assert!( + script.contains(&format!("mkdir -p -- {dest_root}")), + "a missing directory destination must be created: {script}" + ); + assert!( + script.contains(&format!("chown 1000:2000 -- {dest_root}")), + "a created destination must carry the requested ownership: {script}" + ); + assert!( + !script.contains("chmod"), + "--chmod applies to copied content, not to a synthesized destination: {script}" + ); + } + + #[tokio::test] + async fn failed_ownership_lookup_does_not_claim_the_user_is_absent() { + let sandbox = RecordingSandbox { + exit_code: 1, + ..RecordingSandbox::default() + }; + + let err = TemplateStepExecutor::new() + .resolve_ownership(&sandbox, "root:missing-group", "COPY a b") + .await + .expect_err("a failed ownership lookup must fail the step"); + + let message = err.to_string(); + assert!( + message.contains("could not resolve COPY ownership 'root:missing-group'"), + "unexpected message: {message}" + ); + assert!( + !message.contains("does not exist"), + "the same exit also covers a missing group or an unusable lookup: {message}" + ); + } + #[test] fn chown_spec_validation() { assert!(super::is_valid_chown_spec("user"));