feat(sandbox): add E2B-compatible secure sandbox support - #127
Conversation
LSX-s-Software
changed the title
|
We should add HTTPS and API-KEY authentication before merging this PR — without them, an access token alone doesn't provide sufficient security. PR #123 provides a basic draft. |
|
🔍 OpenCodeReview found 10 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| impl Client { | ||
| pub fn files(&self, sandbox_id: &str) -> Result<EnvdFilesClient> { | ||
| EnvdFilesClient::new(&self.base, &self.api_key, sandbox_id) | ||
| let sandbox = self.get_sandbox(sandbox_id)?; |
There was a problem hiding this comment.
[performance · medium]
get_sandbox performs a synchronous ureq network request (with the client's potentially long request timeout). Both current callers invoke files() from async run_async functions, so this blocks a Tokio worker thread and can stall unrelated tasks/progress handling. Make token resolution asynchronous (for example, expose an async files constructor using an async HTTP client or spawn_blocking) or fetch the sandbox metadata before entering the runtime.
Suggestion:
| let sandbox = self.get_sandbox(sandbox_id)?; | |
| // Resolve sandbox metadata asynchronously before constructing this client. |
| } | ||
|
|
||
| match envd_ready_probe(client.clone(), sandbox_id.clone()).await { | ||
| match envd_ready_probe(Arc::clone(&transport)).await { |
There was a problem hiding this comment.
[performance · low]
The readiness probe does not retain or share ownership of the transport, so cloning the Arc on every watchdog iteration adds an unnecessary atomic reference-count operation. Borrow the transport for the duration of the awaited probe instead (envd_ready_probe(&transport) and async fn envd_ready_probe(transport: &Transport)).
Suggestion:
| match envd_ready_probe(Arc::clone(&transport)).await { | |
| match envd_ready_probe(&transport).await { |
| let rest: Vec<String> = cmd_iter.collect(); | ||
|
|
||
| let transport = client.transport(&args.sandbox_id)?; | ||
| let sandbox = client.get_sandbox(&args.sandbox_id)?; |
There was a problem hiding this comment.
[performance · medium]
get_sandbox performs a synchronous HTTP request (ureq::Request::call) from inside this async function, which can block the Tokio runtime thread while the control-plane request is in flight. Fetch the sandbox before entering block_on, or run this call via spawn_blocking (using an owned/cloned client and ID) so unrelated async work is not stalled.
Suggestion:
| let sandbox = client.get_sandbox(&args.sandbox_id)?; | |
| let sandbox_id = args.sandbox_id.clone(); | |
| let sandbox = tokio::task::spawn_blocking({ | |
| let client = client.clone(); | |
| move || client.get_sandbox(&sandbox_id) | |
| }) | |
| .await??; |
| base_url: base_url.trim_end_matches('/').to_string(), | ||
| api_key: api_key.to_string(), | ||
| sandbox_id: sandbox_id.to_string(), | ||
| envd_access_token: envd_access_token.map(str::to_owned), |
There was a problem hiding this comment.
[other · low]
Validate the token as an HTTP header value in Transport::new, as the files client already does. Currently a token containing invalid header bytes (for example, a newline) is accepted by this fallible constructor and only surfaces later as a generic request-builder error on every RPC. Storing a validated HeaderValue also avoids repeating conversion for each request and allows an actionable invalid envd access token header value context at the API boundary.
Suggestion:
| envd_access_token: envd_access_token.map(str::to_owned), | |
| envd_access_token: envd_access_token | |
| .map(reqwest::header::HeaderValue::from_str) | |
| .transpose() | |
| .context("invalid envd access token header value")?, |
| let child_token = orchestrator | ||
| .get_envd_access_token(child) | ||
| .expect("secure child has a token"); | ||
| assert_ne!(child_token, source_token); | ||
| assert!(!child_tokens.contains(&child_token)); | ||
| child_tokens.push(child_token); |
There was a problem hiding this comment.
[test · medium]
These assertions only recompute a token from each child's metadata ID; they do not verify that the same token was supplied to the forked backend/envd. The test would still pass if fork_sandbox_inner passed None, the source token, or another incorrect token in SandboxForkSpec, because MockSandboxBackend::fork currently ignores the specs. Capture the received SandboxForkSpecs in the mock behavior and assert that each successful child's runtime token equals get_envd_access_token(child) (and that the source remains distinct).
Suggestion:
| let child_token = orchestrator | |
| .get_envd_access_token(child) | |
| .expect("secure child has a token"); | |
| assert_ne!(child_token, source_token); | |
| assert!(!child_tokens.contains(&child_token)); | |
| child_tokens.push(child_token); | |
| let child_token = orchestrator | |
| .get_envd_access_token(child) | |
| .expect("secure child has a token"); | |
| assert_ne!(child_token, source_token); | |
| assert!(!child_tokens.contains(&child_token)); | |
| // Also assert that MockSandboxBackend captured this token in the | |
| // SandboxForkSpec used to start this child. | |
| child_tokens.push(child_token); |
| let mut request = envd::reqwest::Client::new() | ||
| .post(format!("http://{}:{port}/process.Process/List", target.ip)); |
There was a problem hiding this comment.
[test · medium]
The unauthorized cases use a raw HTTP/1.1 Connect-style request, but the only successful case uses ProcessClient, which normally reaches envd over tonic/gRPC. This does not prove that a valid token is accepted by the same transport used for the rejection checks (and by Connect clients such as aenv); a Connect handler that rejects every request would still pass. Send a valid-token request through this helper as well and assert a successful status/body, or exercise both missing/wrong/valid tokens through one client stack.
| secure: false, | ||
| }) | ||
| .await?; | ||
| assert_eq!(relaunched.state, SandboxState::Running); |
There was a problem hiding this comment.
[test · medium]
This snapshot relaunch explicitly selects insecure mode, but the test only checks state/proxy-route availability. It never verifies that envd remains callable without X-Access-Token, so an implementation that accidentally requires authentication for all sandboxes—or retains secure auth state in a captured snapshot—could pass while breaking insecure snapshot workloads. Resolve the proxy target and assert that an unauthenticated process-list request succeeds after relaunch (ideally also before capture).
I agree that TLS is required when AgentENV is exposed over an untrusted network, but I don’t think #123 should block this PR. #123 is still a draft, and its token model currently has several unresolved issues:
These are separate authentication layers and should remain separate:
The safer ordering is to merge the correct envd authentication contract in this PR first, then rebase #123 and implement API-key and traffic authentication on top without conflating the credentials. TLS can be handled independently at the deployment or ingress layer. |
a6b8cdd to
8fb985f
Compare
| let sandbox = client.get_sandbox(&args.sandbox_id)?; | ||
| let transport = client.transport(&args.sandbox_id, sandbox.envd_access_token.as_deref())?; |
There was a problem hiding this comment.
[performance · medium]
get_sandbox performs a synchronous ureq network call (with a timeout of up to 120 seconds) inside run_async, which runs on the CLI's current-thread Tokio runtime. This can block the runtime and prevent async cancellation or other runtime work from progressing. Fetch the sandbox before entering block_on and pass its access token into run_async, or provide an async HTTP implementation for this request.
Suggestion:
| let sandbox = client.get_sandbox(&args.sandbox_id)?; | |
| let transport = client.transport(&args.sandbox_id, sandbox.envd_access_token.as_deref())?; | |
| let transport = client.transport(&args.sandbox_id, envd_access_token.as_deref())?; |
| fn validate_explicit_seed(seed: &str) -> Result<&str> { | ||
| let seed = seed.trim(); | ||
| if seed.is_empty() { | ||
| bail!("[sandbox].access_token_hash_seed must be non-empty when configured"); | ||
| } | ||
| Ok(seed) | ||
| } |
There was a problem hiding this comment.
[security · high]
This accepts low-entropy values such as "a" as the HMAC key. Since tokens are deterministic over observable sandbox IDs, anyone who obtains one token can test candidate seeds offline and then derive tokens for other IDs. Require a cryptographically strong representation here (for example, exactly 64 hex characters decoding to 32 bytes) and use the decoded bytes as the HMAC key; update placeholder examples accordingly.
| let mut request = envd::reqwest::Client::new() | ||
| .post(format!("http://{}:{port}/process.Process/List", target.ip)); |
There was a problem hiding this comment.
[test · medium]
The negative authentication probe is not a valid Connect/gRPC request: it omits the protocol content type/version and encoded ListRequest body. Therefore, the observed 401 can come from handling a malformed request before the behavior of a real envd client is exercised, allowing a valid unauthenticated RPC regression to go undetected. Use ProcessClient::connect(..., None/Some("wrong-token")), invoke list, and assert that the returned RPC status is unauthenticated.
8fb985f to
244e2a5
Compare
| impl Client { | ||
| pub fn files(&self, sandbox_id: &str) -> Result<EnvdFilesClient> { | ||
| EnvdFilesClient::new(&self.base, &self.api_key, sandbox_id) | ||
| let sandbox = self.get_sandbox(sandbox_id)?; |
There was a problem hiding this comment.
[performance · medium]
This introduces blocking ureq network I/O inside Client::files. Both current callers invoke files() from run_async on the CLI's single-threaded Tokio runtime, so this request blocks the runtime (and cannot be cancelled) for up to the agent timeout. Keep this constructor free of I/O by passing the already-fetched access token into it, or fetch sandbox details before entering block_on; alternatively, use an async HTTP client and make this API async.
| } | ||
|
|
||
| match envd_ready_probe(client.clone(), sandbox_id.clone()).await { | ||
| match envd_ready_probe(Arc::clone(&transport)).await { |
There was a problem hiding this comment.
[performance · low]
The probe only borrows the transport while awaiting ready(), so cloning the Arc on every watchdog iteration is unnecessary. Accept &Transport (or &Arc<Transport>) in envd_ready_probe and pass transport.as_ref() here; this avoids repeated atomic reference-count updates and expresses the ownership requirement more clearly.
Suggestion:
| match envd_ready_probe(Arc::clone(&transport)).await { | |
| match envd_ready_probe(transport.as_ref()).await { |
| base_url: String, | ||
| api_key: String, | ||
| sandbox_id: String, | ||
| envd_access_token: Option<String>, |
There was a problem hiding this comment.
[security · medium]
Store this secret as a sensitive HeaderValue, rather than a plain String. The current .header("X-Access-Token", token) conversion creates a non-sensitive value, so request/header diagnostics may expose the token; it also defers invalid-header errors until the request is built or sent even though new already returns Result. Parse and mark it sensitive in new, as the files client does.
Suggestion:
| envd_access_token: Option<String>, | |
| envd_access_token: Option<reqwest::header::HeaderValue>, |
| if target_port != ConfigManager::global_config().tools.control_plane_port { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
[security · medium]
This compares against the current process-global port, not the paused sandbox's effective envd port. FirecrackerCommonConfig.control_plane_port is persisted in paused state and can differ after a configuration change/restart. In that case, a request targeting the sandbox's old envd port skips this check and can trigger an unauthenticated auto-resume (envd will reject the eventual request, but the protected resume side effect has already occurred). Persist/expose the effective control-plane port in sandbox metadata and compare against that value, or otherwise ensure secure envd auto-resume authorization cannot be bypassed by config drift.
Suggestion:
| if target_port != ConfigManager::global_config().tools.control_plane_port { | |
| return Ok(()); | |
| } | |
| if target_port != metadata.control_plane_port { | |
| return Ok(()); | |
| } |
| if let Some(seed) = config.sandbox.access_token_hash_seed.as_deref() { | ||
| return Self::new(seed); | ||
| } |
There was a problem hiding this comment.
[bug · medium]
This silently replaces an existing managed seed whenever an explicit cluster seed is configured. Persisted secure sandboxes were initialized with tokens derived from the old managed seed; after restart, the orchestrator derives a new token while the resumed envd state still authenticates with the old one, so recovery/access fails. This is especially likely during the documented migration to cross-node recovery. Detect an existing managed seed and reject a mismatched explicit seed, or persist/version a seed fingerprint and provide an explicit migration/rotation path.
| fn validate_explicit_seed(seed: &str) -> Result<&str> { | ||
| let seed = seed.trim(); | ||
| if seed.is_empty() { | ||
| bail!("[sandbox].access_token_hash_seed must be non-empty when configured"); | ||
| } | ||
| Ok(seed) | ||
| } |
There was a problem hiding this comment.
[security · high]
The configured value is the HMAC authentication key, but this validation accepts low-entropy values such as "a" (and the sample configuration only says “secret”). Since sandbox IDs and derived tokens can be known to clients, a weak key can be tested offline and then used to forge tokens for other sandbox IDs. Require sufficient key material—preferably a strict encoding of at least 32 random bytes—and update the diagnostic/config guidance accordingly.
| assert_eq!( | ||
| metadata.access_token_hash, | ||
| hash_access_token(token.expose()) | ||
| ); |
There was a problem hiding this comment.
[test · medium]
This assertion computes the expected value with the same helper used by set_access_token, so it will still pass if the digest algorithm, byte encoding, or hex representation changes incorrectly. Since envd authentication depends on this wire-format contract, add a fixed SHA-512 test vector (for example, assert the exact lowercase digest of "abc") and retain this assertion only to verify token plumbing. That will catch compatibility-breaking hash changes before all secure envd requests are rejected.
Suggestion:
| assert_eq!( | |
| metadata.access_token_hash, | |
| hash_access_token(token.expose()) | |
| ); | |
| assert_eq!( | |
| hash_access_token("abc"), | |
| concat!( | |
| "ddaf35a193617abacc417349ae204131", | |
| "12e6fa4e89a97ea20a9eeee64b55d39a", | |
| "2192992a274fc1a836ba3c23a3feebbd", | |
| "454d4423643ce80e2a9ac94fa54ca49f" | |
| ) | |
| ); | |
| assert_eq!( | |
| metadata.access_token_hash, | |
| hash_access_token(token.expose()) | |
| ); |
| Self::from_snapshot_config_with_override( | ||
| snapshot.clone(), | ||
| SandboxId::new(), | ||
| snapshot.common.envd_access_token.clone(), | ||
| ) |
There was a problem hiding this comment.
[security · high]
This creates a new runtime identity but reuses the source sandbox's credential. SandboxAccessTokenGenerator derives credentials from SandboxId, so the resulting sandbox has a token that does not belong to its new ID; repeated restores also share the same bearer token. This can break ID-based validation and allows one restored sandbox's credential to authenticate to its siblings. Make callers provide the token for the new ID (or accept a generator/spec), rather than copying it from the source snapshot; if this API is intended to resume the same identity, preserve the source ID instead.
Suggestion:
| Self::from_snapshot_config_with_override( | |
| snapshot.clone(), | |
| SandboxId::new(), | |
| snapshot.common.envd_access_token.clone(), | |
| ) | |
| let id = SandboxId::new(); | |
| Self::from_snapshot_config_with_override(snapshot.clone(), id, None) |
| let uri: Uri = addr.parse().context("Invalid URI")?; | ||
| let access_token = access_token | ||
| .map(|token| { | ||
| let mut token = http::HeaderValue::from_str(token)?; |
There was a problem hiding this comment.
[bug · high]
This does not compile because from_str is provided by the std::str::FromStr trait, which is not imported in this module. Import that trait or parse the value directly.
Suggestion:
| let mut token = http::HeaderValue::from_str(token)?; | |
| let mut token = token.parse::<http::HeaderValue>()?; |
yingdi-shan
left a comment
yingdi-shan
left a comment
There was a problem hiding this comment.
As we discussed before, I think we should avoid adding a shared seed in this PR. It only serves as a workaround for VM migration, which isn't on the near-term roadmap. Removing it here would simplify deployment.
|
The seed is already OPTIONAL. We are not requiring users to configure it or enabling cross-node migration in this PR. This only documents an optional way for operators to prepare for future multi-node sandbox recovery, so they won’t need to reconfigure the entire cluster later. In practice, configuring the seed takes only a few commands, which is negligible compared with the setup already required for a multi-node deployment. Users who do not need this can simply leave the seed unset and use the automatically managed node-local seed. |
The docs does not say that. Maybe you should update the docs. |
| impl Client { | ||
| pub fn files(&self, sandbox_id: &str) -> Result<EnvdFilesClient> { | ||
| EnvdFilesClient::new(&self.base, &self.api_key, sandbox_id) | ||
| let sandbox = self.get_sandbox(sandbox_id)?; |
There was a problem hiding this comment.
get_sandbox uses the synchronous ureq client, so this newly added call performs a blocking network request inside upload/download's async run_async task. A slow or unreachable control-plane request can block the Tokio runtime for the ureq timeout (up to 120 seconds), delaying unrelated async work. Resolve the token before entering the async runtime, or provide/use an async sandbox-detail request instead.
Suggestion:
| let sandbox = self.get_sandbox(sandbox_id)?; | |
| let sandbox = self.get_sandbox_async(sandbox_id).await?; |
| let rest: Vec<String> = cmd_iter.collect(); | ||
|
|
||
| let transport = client.transport(&args.sandbox_id)?; | ||
| let sandbox = client.get_sandbox(&args.sandbox_id)?; |
There was a problem hiding this comment.
get_sandbox performs a synchronous ureq network request, but this function is running inside Tokio's async runtime. A slow or stalled API response can block the runtime thread for the client's timeout (up to 120 seconds), delaying the subsequent envd stream and any other runtime work. Fetch the sandbox detail before entering run_async, or execute this blocking request via spawn_blocking/an async HTTP client.
| .header("Connect-Protocol-Version", "1") | ||
| .header("Connect-Protocol-Version", "1"); | ||
| match &self.envd_access_token { | ||
| Some(token) => builder.header("X-Access-Token", token), |
There was a problem hiding this comment.
The access token is inserted from a String, so the resulting HeaderValue is not marked sensitive (unlike the multipart/files client above). If a Connect request or its headers is formatted with Debug during error handling or tracing, this credential can be exposed. Store a pre-built HeaderValue with set_sensitive(true) (validating it in new) and pass that value here.
Suggestion:
| Some(token) => builder.header("X-Access-Token", token), | |
| Some(token) => builder.header("X-Access-Token", token), |
| const TARGET_PORT_HEADER: &str = "x-agentenv-target-port"; | ||
| /// E2B-compatible alias for the target port header. | ||
| const E2B_TARGET_PORT_HEADER: &str = "e2b-sandbox-port"; | ||
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; |
There was a problem hiding this comment.
x-access-token is left in the client headers and sanitize_request_headers does not remove it, so this credential is forwarded to every proxy target, not only envd's control-plane port. The added test at the arbitrary upstream_addr.port() confirms this behavior. A caller can attach the sandbox's envd token while selecting another sandbox service port, disclosing the credential to that service (and its logs/application). Strip this header for non-control-plane targets (while retaining it only for the authenticated envd control-plane request).
Suggestion:
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; | |
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; |
| if config.sandbox.access_token_hash_seed.is_none() | ||
| && config.cluster.scheduler_endpoint.is_some() | ||
| { | ||
| warn!( | ||
| path = %managed_seed_path.display(), | ||
| "using a node-local managed envd access-token seed; configure AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED with the same value on every node before enabling cross-node sandbox recovery" | ||
| ); | ||
| } |
There was a problem hiding this comment.
When no explicit seed is configured, this intentionally creates/uses a node-local seed even if scheduler_endpoint is set; the warning does not prevent startup. Consequently, two nodes can derive different bearer tokens for the same secure sandbox, so cross-node recovery/proxying fails after placement or restart. If cross-node operation is supported in this mode, require a shared configured seed (or shared secret storage) at startup instead of allowing an informational warning.
| use crate::types::SandboxId; | ||
|
|
||
| const EMPTY_ACCESS_TOKEN: &str = ""; | ||
| const RESERVED_FIELDS: [&str; 4] = ["instanceID", "envID", "address", "accessTokenHash"]; |
There was a problem hiding this comment.
This reserved-key list duplicates the serialized field names declared above. Adding or renaming an MMDS runtime field requires remembering to update this array; otherwise with_extra can expose a newly reserved field to untrusted opaque metadata. Consider centralizing the canonical field-name definition or adding a test that enumerates every runtime field against the filter.
| pub(crate) fn with_access_token(mut self, token: Option<&EnvdAccessToken>) -> Self { | ||
| self.set_access_token(token); | ||
| self | ||
| } |
There was a problem hiding this comment.
The runtime token is not guaranteed to be reflected in the MMDS payload at the point it is sent. mmds_metadata() in sandbox.rs returns the stored metadata unchanged and, when it is absent, creates MmdsMetadata::new(...) with the empty-token hash; it never calls set_access_token using common.envd_access_token. Consequently, any resume/configuration path with a runtime token but missing or stale metadata sends an empty/stale accessTokenHash, while envd requires the generated token. Please synchronize the metadata with the runtime token immediately before each set_mmds (or make mmds_metadata() apply common.envd_access_token).
Suggestion:
| pub(crate) fn with_access_token(mut self, token: Option<&EnvdAccessToken>) -> Self { | |
| self.set_access_token(token); | |
| self | |
| } | |
| pub(crate) fn with_access_token(mut self, token: Option<&EnvdAccessToken>) -> Self { | |
| self.set_access_token(token); | |
| self | |
| } |
| Self::from_snapshot_config_with_override( | ||
| snapshot.clone(), | ||
| SandboxId::new(), | ||
| snapshot.common.envd_access_token.clone(), | ||
| ) |
There was a problem hiding this comment.
SandboxAccessTokenGenerator binds each token to the sandbox ID, but this default restoration path generates a new ID while copying the source snapshot's token. When a secure snapshot is passed here (for example, an in-memory snapshot config), EnvD will receive credentials generated for the original sandbox and MMDS will advertise the new sandbox's token hash, so authenticated requests fail; if token validation is ID-independent at any boundary, this also enables credential reuse. Derive/pass a token for id (or preserve the source ID) instead of inheriting the source token when assigning a new identity.
| std::env::set_var( | ||
| "AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED", | ||
| "integration-test-seed", | ||
| ); |
There was a problem hiding this comment.
This mutates the process-wide environment from an async test helper that can be called concurrently by multiple integration tests. Besides leaking the test seed into all tests in the process (including tests that exercise missing/invalid configuration), environment mutation is not a generally thread-safe way to coordinate per-test configuration. Set this once in test-process initialization (before any test threads start), or inject the seed through the test configuration instead of changing the global environment here.
| let uri: Uri = addr.parse().context("Invalid URI")?; | ||
| let access_token = access_token | ||
| .map(|token| { | ||
| let mut token = http::HeaderValue::from_str(token)?; |
There was a problem hiding this comment.
This does not compile because from_str is provided by the std::str::FromStr trait, which is not imported in this module. Import that trait or avoid the required import by parsing the string explicitly.
Suggestion:
| let mut token = http::HeaderValue::from_str(token)?; | |
| let mut token = token.parse::<http::HeaderValue>()?; |
0a238fb to
30340ae
Compare
| let sandbox = self.get_sandbox(sandbox_id)?; | ||
| EnvdFilesClient::new( |
There was a problem hiding this comment.
get_sandbox performs a synchronous ureq network request, but files() is called from the async upload/download paths. This newly introduced call therefore blocks the Tokio runtime thread for up to the client's connect/request timeout, potentially stalling other async work (and is especially problematic on a single-thread runtime). Fetch the sandbox detail before entering run_async, or execute this synchronous lookup via spawn_blocking/provide an async client method.
| async fn envd_ready_probe(transport: Arc<Transport>) -> Result<bool> { | ||
| match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await { |
There was a problem hiding this comment.
The probe only borrows Transport, so taking an Arc by value forces an unnecessary atomic refcount increment on every watchdog iteration (Arc::clone(&transport)). Accept &Transport instead and call it as envd_ready_probe(&transport); the borrow remains valid for the awaited call because the watchdog owns the Arc.
Suggestion:
| async fn envd_ready_probe(transport: Arc<Transport>) -> Result<bool> { | |
| match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await { | |
| async fn envd_ready_probe(transport: &Transport) -> Result<bool> { | |
| match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await { |
| let rest: Vec<String> = cmd_iter.collect(); | ||
|
|
||
| let transport = client.transport(&args.sandbox_id)?; | ||
| let sandbox = client.get_sandbox(&args.sandbox_id)?; |
There was a problem hiding this comment.
get_sandbox performs a synchronous ureq network request, but this call is now made inside run_async before the first .await. That blocks the Tokio executor for the request timeout (up to the configured 120 seconds), and would stall any other work sharing this runtime. Fetch the sandbox/token before entering block_on, use an async HTTP client, or move this blocking operation to tokio::task::spawn_blocking.
Suggestion:
| let sandbox = client.get_sandbox(&args.sandbox_id)?; | |
| let sandbox = tokio::task::spawn_blocking({ | |
| let client = client.clone(); | |
| let sandbox_id = args.sandbox_id.clone(); | |
| move || client.get_sandbox(&sandbox_id) | |
| }) | |
| .await??; |
| .header("Connect-Protocol-Version", "1") | ||
| .header("Connect-Protocol-Version", "1"); | ||
| match &self.envd_access_token { | ||
| Some(token) => builder.header("X-Access-Token", token), |
There was a problem hiding this comment.
The access token is inserted from a plain String, so the resulting HeaderValue is not marked sensitive. Any request/header debug output (including middleware diagnostics) may therefore expose this credential. Parse and validate the token in Transport::new, call set_sensitive(true), and store the resulting HeaderValue for reuse, as the other envd clients do.
Suggestion:
| Some(token) => builder.header("X-Access-Token", token), | |
| Some(token) => builder.header("X-Access-Token", token), // where token is a prevalidated HeaderValue with set_sensitive(true) |
| const TARGET_PORT_HEADER: &str = "x-agentenv-target-port"; | ||
| /// E2B-compatible alias for the target port header. | ||
| const E2B_TARGET_PORT_HEADER: &str = "e2b-sandbox-port"; | ||
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; |
There was a problem hiding this comment.
This credential is accepted from every proxy request, but sanitize_request_headers and the WebSocket path do not remove it, so it is forwarded to any client-selected target port (the added test explicitly verifies forwarding to an arbitrary upstream). A caller can therefore disclose the sandbox's envd token to an application service or other untrusted workload by including this header, and the token is also exposed on non-control-plane routes where no authorization check is performed. Strip this header by default and only forward it when the upstream is the trusted envd control-plane endpoint (or otherwise ensure it cannot be used for arbitrary target ports).
Suggestion:
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; | |
| const ENVD_ACCESS_TOKEN_HEADER: &str = "x-access-token"; |
| fn validate_explicit_seed(seed: &str) -> Result<&str> { | ||
| let seed = seed.trim(); | ||
| if seed.is_empty() { | ||
| bail!("[sandbox].access_token_hash_seed must be non-empty when configured"); | ||
| } | ||
| Ok(seed) | ||
| } |
There was a problem hiding this comment.
The explicit seed path accepts any non-empty string, including short or predictable values such as the documented "replace-with-a-secret" placeholder or a simple test-like value. This seed is the sole HMAC key for every secure sandbox token, so an operator who supplies low-entropy configuration can brute-force the key and forge access to all sandboxes. Enforce a minimum cryptographic strength/length (for example, exactly 32 random bytes represented as 64 lowercase hex characters, matching the managed seed), or reject weak values with an explicit diagnostic.
Suggestion:
| fn validate_explicit_seed(seed: &str) -> Result<&str> { | |
| let seed = seed.trim(); | |
| if seed.is_empty() { | |
| bail!("[sandbox].access_token_hash_seed must be non-empty when configured"); | |
| } | |
| Ok(seed) | |
| } | |
| fn validate_explicit_seed(seed: &str) -> Result<&str> { | |
| let seed = seed.trim(); | |
| if !is_valid_managed_seed(seed) { | |
| bail!( | |
| "[sandbox].access_token_hash_seed must contain exactly {SEED_HEX_LEN} lowercase hexadecimal characters" | |
| ); | |
| } | |
| Ok(seed) | |
| } |
| fs::create_dir_all(parent) | ||
| .with_context(|| format!("create managed secret directory {}", parent.display()))?; | ||
| validate_managed_seed_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; |
There was a problem hiding this comment.
When this directory is newly created, create_dir_all applies the process umask/default mode first and only then changes it to 0700. During that interval another local user/process with access to AENV_HOME can potentially traverse the directory; although short, the seed temporary file is created later and the HMAC key protects every sandbox token. Create the directory with restrictive permissions atomically (or otherwise ensure the parent hierarchy is private before exposing the seed path), then validate its identity and mode.
Suggestion:
| fs::create_dir_all(parent) | |
| .with_context(|| format!("create managed secret directory {}", parent.display()))?; | |
| validate_managed_seed_directory_identity(parent).with_context(|| { | |
| format!( | |
| "validate managed secret directory ownership {}", | |
| parent.display() | |
| ) | |
| })?; | |
| set_permissions(parent, 0o700)?; | |
| fs::create_dir_all(parent) | |
| .with_context(|| format!("create managed secret directory {}", parent.display()))?; | |
| set_permissions(parent, 0o700)?; | |
| validate_managed_seed_directory_identity(parent).with_context(|| { | |
| format!( | |
| "validate managed secret directory ownership {}", | |
| parent.display() | |
| ) | |
| })?; |
| Self::from_snapshot_config_with_override( | ||
| snapshot.clone(), | ||
| SandboxId::new(), | ||
| snapshot.common.envd_access_token.clone(), | ||
| ) |
There was a problem hiding this comment.
This assigns a new sandbox identity while reusing the source sandbox's credential. Because access tokens are derived per SandboxId, an in-memory secure snapshot resumed through this public path will create a different sandbox that is still controllable with the source token; any identity-based token validation will also derive a different token and disagree with envd. Do not carry a token across an identity change. Require the caller to provide a token generated for the new ID (as the orchestrator/factory path does), or preserve the original identity when this is an in-place resume.
Suggestion:
| Self::from_snapshot_config_with_override( | |
| snapshot.clone(), | |
| SandboxId::new(), | |
| snapshot.common.envd_access_token.clone(), | |
| ) | |
| let id = SandboxId::new(); | |
| Self::from_snapshot_config_with_override(snapshot.clone(), id, None) |
| std::env::set_var( | ||
| "AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED", | ||
| "integration-test-seed", | ||
| ); |
There was a problem hiding this comment.
setup_runtime_only() runs from #[tokio::test] after the multithreaded runtime has started, and multiple integration tests can invoke it concurrently. Mutating the process environment at that point is not safe on Unix when any other thread may read the environment (this is why set_var is unsafe in Rust 2024), and it can also overwrite a seed explicitly supplied by the test environment. Please inject this value through a test configuration/init API, or arrange for the test runner to set it before the process becomes multithreaded rather than changing it here.
Suggestion:
| std::env::set_var( | |
| "AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED", | |
| "integration-test-seed", | |
| ); | |
| // Supply the integration-test seed through ConfigManager test initialization | |
| // instead of mutating the process environment from the async test runtime. |
| let uri: Uri = addr.parse().context("Invalid URI")?; | ||
| let access_token = access_token | ||
| .map(|token| { | ||
| let mut token = http::HeaderValue::from_str(token)?; |
There was a problem hiding this comment.
This does not compile because from_str is provided by std::str::FromStr, which is not imported in this module. Import the trait or use str::parse so the token validation remains fallible.
Suggestion:
| let mut token = http::HeaderValue::from_str(token)?; | |
| let mut token = token.parse::<http::HeaderValue>()?; |
2 participants
What
Introduce end-to-end support for E2B-compatible secure sandboxes. When a sandbox is created with
secure: true, AgentENV generates a sandbox-specific envd access token and enforces it across the complete sandbox lifecycle.securesandbox create field and returnenvdAccessTokenfrom create, get, connect, and fork responses.Why
AgentENV did not previously implement the E2B
securesandbox contract. Current E2B SDKs create secure sandboxes by default, so compatibility requires AgentENV to return an envd access token and enforce it transparently for SDK and CLI operations.Introducing secure support should not force existing or single-node users to add configuration before upgrading. AgentENV therefore creates and reuses a managed local seed when no explicit seed is supplied, while multi-node operators can configure one shared seed through their deployment secret mechanism.
Related issue
N/A - no linked issue.
Scope and non-goals
In E2B semantics,
secureauthenticates envd control communication. This PR does not add authentication to arbitrary application ports or implement cross-node sandbox migration. The install script remains unchanged.Design and behavior changes
/initreceives the token in bothX-Access-Tokenand the request body, matching envd's contract.false, so legacy sandbox records continue to restore as non-secure.$AENV_HOME/secrets/sandbox-access-token-hash-seed. The directory is0700, the file is0600, and malformed, unreadable, symlinked, or unexpectedly missing state fails instead of rotating silently.Compatibility and operations
securerequest fields and optionalenvdAccessTokenresponse fields. Omittingsecurepreserves the previous non-secure behavior.[sandbox].access_token_hash_seed/AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED; single-node startup auto-generates a persistent seed when unset.securewith a legacy default offalse.$AENV_HOME/secrets/sandbox-access-token-hash-seedonce secure sandboxes exist; deleting or changing it invalidates their tokens.agentenv-runtime-secretsSecret; the local-dev overlay provides a test-only value.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetSkipped checks and reasons:
make -C services testwas not run becauseservices/is unchanged.Risks and reviewer notes
src/sandbox/access.rsfor managed-secret durability, permissions, and concurrent startup behavior.Checklist