feat: add single-tenant API key authentication - #123
Conversation
|
Adding a production deployment data point, since this is a design-review vehicle and it may be useful to know how the current "no auth" state actually gets handled in the field. We run AgentENV as the isolation layer under a multi-user internal agent platform — each end user gets one persistent sandbox, and an application-side shim maps users to sandbox IDs. Because there is no authentication today, we follow the README's guidance literally: the API is bound to Three observations that bear on the design: 1. Loopback-only is a real deployment posture, not just a stopgap — please keep it viable. For us the shared key would be defence in depth behind the loopback bind, not the primary boundary. If the implementation ends up requiring a key even for a loopback-only single-node install, that is a small friction on a config that is already safe; an "auth required unless bound to loopback" default, or simply generating the key automatically as you describe, avoids making people opt out of security to keep a working setup. The automatic generation in your Scope section reads like it already handles this well. 2. The derived sandbox-scoped token is the part we would use most. Our shim already knows which user owns which sandbox; AgentENV does not, and deliberately so. A token scoped to a sandbox ID means a compromised component can only reach the sandbox it was issued for, rather than the whole node. That is a meaningful reduction in blast radius for anyone running this multi-tenant on top, and it is the piece a shared key alone does not give you. 3. Key rotation is the operational question we would ask first. With a single shared key persisted at setup time, what does rotation look like for a running node with live sandboxes — is the expectation a restart, or can the key be re-read? If derived sandbox tokens are a function of the shared key, does rotating it invalidate every in-flight sandbox token? For long-lived sandboxes (ours persist across a working session and pause/resume rather than being recreated per command) that distinction matters quite a bit. Worth documenting whichever way it lands. One smaller note: if the key ends up in a config file written by the setup paths, it would be helpful for the docs to state the expected file mode and owner explicitly. It is the kind of thing that is obvious to whoever writes it and non-obvious to whoever inherits the box. Happy to test a branch against a real multi-user deployment if that is useful — we have a node running this pattern daily and can report back on anything that breaks under pause/resume or long-lived sandboxes. |
31276fd to
636dc52
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Err(error) if error.error.kind() == io::ErrorKind::AlreadyExists => { | ||
| read(path).context("load concurrently generated API key") | ||
| } |
There was a problem hiding this comment.
Fixed in d7d47f9. Managed API keys now use the shared managed-secret loader: it validates that the secrets directory is a non-symlink directory owned by the effective uid with mode 0700, opens the key with O_NOFOLLOW | O_NONBLOCK, and validates the opened object is a regular file owned by the effective uid with mode 0600 before reading it. The AlreadyExists fallback revalidates the directory and goes through the same no-follow open and file validation. External orchestrator-provided secrets remain unchanged.
| Err(error) if error.error.kind() == io::ErrorKind::AlreadyExists => { | ||
| read(path).context("load concurrently generated API key") | ||
| } |
There was a problem hiding this comment.
Fixed in d7d47f9. Managed API keys now use the shared managed-secret loader: it validates that the secrets directory is a non-symlink directory owned by the effective uid with mode 0700, opens the key with O_NOFOLLOW | O_NONBLOCK, and validates the opened object is a regular file owned by the effective uid with mode 0600 before reading it. The AlreadyExists fallback revalidates the directory and goes through the same no-follow open and file validation. External orchestrator-provided secrets remain unchanged.
| shift | ||
| KUBECTL_BIN="${KUBECTL:-kubectl}" | ||
| OVERLAY_NAME="${K8S_OVERLAY:-default}" | ||
| NAMESPACE="${K8S_NAMESPACE:-agentenv-system}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in amended commit b6bedef. The temporary Kustomization, Namespace resource, and scheduler discovery config now use the validated K8S_NAMESPACE value, so preflight, generated resources, and scheduler registration agree.
| export E2B_SANDBOX_URL="${AENV_PROXY_URL}" | ||
| export E2B_API_KEY="e2b_000000" | ||
| export E2B_ACCESS_TOKEN="${AENV_API_KEY}" | ||
| export E2B_API_KEY="${AENV_API_KEY}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if let Some((sandbox_id, target_port)) = | ||
| proxy::route_for_auth(&request, api_impl.sandbox_proxy_domains()) | ||
| { |
There was a problem hiding this comment.
Fixed in amended commit b6bedef. Explicit /proxy routes now derive the auth identity from routing headers, matching proxy dispatch; host routing is used only for non-explicit routes.
| match parse_host_proxy_route(request_host(request), domains) { | ||
| Ok(Some(route)) => return Some((route.sandbox_id, route.target_port)), | ||
| Err(_) => return None, | ||
| Ok(None) => {} | ||
| } | ||
|
|
||
| Some(( | ||
| parse_sandbox_id_header(request.headers()).ok()?, | ||
| parse_target_port_header(request.headers()).ok()?, | ||
| )) |
There was a problem hiding this comment.
Fixed in amended commit b6bedef. Host-route parse failures and absent host routes now fall back to explicit routing headers, while valid host routes retain precedence for host-only requests.
| create_directory(parent)?; | ||
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; |
There was a problem hiding this comment.
Reviewed for amended commit b6bedef. I left the pathname TOCTOU suggestion unchanged: the managed directory must be a non-symlink directory owned by the effective UID with mode 0700, so replacing it requires the same UID or privileged mount capability and already provides equivalent secret access. An openat-based rewrite would add substantial platform-specific complexity without changing that threat model.
| #[cfg(unix)] | ||
| fn set_test_permissions(path: &Path, mode: u32) -> Result<()> { | ||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| fs::set_permissions(path, fs::Permissions::from_mode(mode))?; | ||
| Ok(()) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in amended commit b6bedef. Added a cfg(not(unix)) no-op test helper so the access tests compile on non-Unix targets.
d7d47f9 to
b6bedef
Compare
| shift | ||
| KUBECTL_BIN="${KUBECTL:-kubectl}" | ||
| OVERLAY_NAME="${K8S_OVERLAY:-default}" | ||
| NAMESPACE="${K8S_NAMESPACE:-agentenv-system}" |
There was a problem hiding this comment.
Fixed in amended commit b6bedef. The temporary Kustomization, Namespace resource, and scheduler discovery config now use the validated K8S_NAMESPACE value, so preflight, generated resources, and scheduler registration agree.
| match parse_host_proxy_route(request_host(request), domains) { | ||
| Ok(Some(route)) => return Some((route.sandbox_id, route.target_port)), | ||
| Err(_) => return None, | ||
| Ok(None) => {} | ||
| } | ||
|
|
||
| Some(( | ||
| parse_sandbox_id_header(request.headers()).ok()?, | ||
| parse_target_port_header(request.headers()).ok()?, | ||
| )) |
There was a problem hiding this comment.
Fixed in amended commit b6bedef. Host-route parse failures and absent host routes now fall back to explicit routing headers, while valid host routes retain precedence for host-only requests.
| if let Some((sandbox_id, target_port)) = | ||
| proxy::route_for_auth(&request, api_impl.sandbox_proxy_domains()) | ||
| { |
There was a problem hiding this comment.
Fixed in amended commit b6bedef. Explicit /proxy routes now derive the auth identity from routing headers, matching proxy dispatch; host routing is used only for non-explicit routes.
| create_directory(parent)?; | ||
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; |
There was a problem hiding this comment.
Reviewed for amended commit b6bedef. I left the pathname TOCTOU suggestion unchanged: the managed directory must be a non-symlink directory owned by the effective UID with mode 0700, so replacing it requires the same UID or privileged mount capability and already provides equivalent secret access. An openat-based rewrite would add substantial platform-specific complexity without changing that threat model.
|
|
||
| cp -R "${SCRIPT_DIR}" "${TEMP_DIR}/k8s" | ||
| cp "${REPO_ROOT}/config/default.toml" "${TEMP_DIR}/k8s/base/config/agentenv.toml" | ||
| sed_in_place "s#^namespace: agentenv-system#namespace: ${NAMESPACE}#" "${TEMP_DIR}/k8s/base/kustomization.yaml" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| Read::by_ref(&mut file) | ||
| .take((max_len + 1) as u64) | ||
| .read_to_string(&mut contents) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if let Some(contents) = managed_secret::read(managed_path, MANAGED_SEED_FILE_MAX_LEN)? { | ||
| return validate_managed_seed(managed_path, &contents); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b6bedef to
31eda91
Compare
| render_api_key "${TEMP_DIR}/k8s/base/kustomization.yaml" | ||
| [[ "${restore_xtrace}" == "0" ]] || set -x | ||
| fi |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| fn read_external(path: &Path) -> Result<String, io::Error> { | ||
| let value = read_bounded(File::open(path)?)?; | ||
| validate_file_contents(&value).map_err(io::Error::other) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| observability: Option<Arc<ObservabilityService>>, | ||
| proxy_client: ProxyClient, | ||
| sandbox_proxy_domains: Vec<String>, | ||
| api_key: String, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .layer(middleware::from_fn_with_state( | ||
| api_impl, | ||
| auth::require_auth::<I>, | ||
| )) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| render_api_key "${TEMP_DIR}/k8s/base/kustomization.yaml" | ||
| [[ "${restore_xtrace}" == "0" ]] || set -x | ||
| fi |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| -H "x-agentenv-sandbox-id: ${secure_sandbox_id}" \ | ||
| -H "x-agentenv-target-port: 49983" \ | ||
| "${AENV_PROXY_URL}/health" | ||
| assert_status "$HTTP_STATUS" "401" "secure envd rejects missing token" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| fn validate(value: &str) -> Result<String> { | ||
| if !(32..=API_KEY_MAX_LEN).contains(&value.len()) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .layer(middleware::from_fn_with_state( | ||
| api_impl, | ||
| auth::require_auth::<I>, | ||
| )) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .layer(middleware::from_fn_with_state( | ||
| api_impl, | ||
| auth::require_auth::<I>, | ||
| )) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| pub fn generate_traffic(&self, subject: SandboxId) -> String { | ||
| self.generate_for(format!("{TRAFFIC_ACCESS_TOKEN_PREFIX}-{subject}").as_bytes()) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| : "${AENV_PORT:=18080}" | ||
| : "${AENV_URL:=http://127.0.0.1:${AENV_PORT}}" | ||
| : "${AENV_API_KEY:=e2e-test-key}" | ||
| : "${AENV_API_KEY:=e2b_0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if !has_proxy_prefix(request.uri().path()) { | ||
| if let Ok(Some(route)) = parse_host_proxy_route(request_host(request), domains) { | ||
| return Some((route.sandbox_id, route.target_port)); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| fn control_plane_port(&self) -> Option<u16> { | ||
| Some(self.snapshot_config.common.control_plane_port) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fd45e26 to
5b8e8e2
Compare
| secure_sandbox_id=$(create_sandbox "$AENV_TEMPLATE_ID" 60 \ | ||
| '{"secure":true,"network":{"allowPublicTraffic":false}}'); _sync_http | ||
| if [[ -n "$secure_sandbox_id" ]]; then | ||
| track_sandbox "$secure_sandbox_id" | ||
| fi | ||
| assert_status "$HTTP_STATUS" "201" "create private secure sandbox" | ||
| assert_not_empty "$secure_sandbox_id" "private secure sandbox ID present" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| single_header(headers, API_KEY_HEADER).is_some_and(|value| { | ||
| let candidate = value.as_bytes(); | ||
| let expected = self.api_key.as_bytes(); | ||
| candidate.len() == expected.len() && bool::from(candidate.ct_eq(expected)) | ||
| }) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| pub(crate) fn route_for_auth(request: &Request, domains: &[String]) -> Option<(SandboxId, u16)> { | ||
| if !has_proxy_prefix(request.uri().path()) { | ||
| match parse_host_proxy_route(request_host(request), domains) { | ||
| Ok(Some(route)) => return Some((route.sandbox_id, route.target_port)), | ||
| Ok(None) => {} | ||
| Err(_) => return None, | ||
| } | ||
| } | ||
|
|
||
| Some(( | ||
| parse_sandbox_id_header(request.headers()).ok()?, | ||
| parse_target_port_header(request.headers()).ok()?, | ||
| )) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| Ok(None) => { | ||
| return next.run(request).await; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
5b8e8e2 to
32bf340
Compare
| if [[ "${MODE}" != "delete" ]]; then | ||
| restore_xtrace=0 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if [[ "${AENV_API_KEY+x}" == "x" ]]; then | ||
| if [[ -z "${AENV_API_KEY}" ]]; then | ||
| echo "AENV_API_KEY must not be empty" >&2 | ||
| exit 1 | ||
| fi | ||
| API_KEY_VALUE="${AENV_API_KEY}" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| let metadata = match api_impl.orchestrator().get_sandbox(&sandbox_id).await { | ||
| Ok(Some(metadata)) => metadata, | ||
| Ok(None) => { | ||
| request.headers_mut().remove(ENVD_ACCESS_TOKEN_HEADER); | ||
| return proxy::sandbox_not_found_response(sandbox_id); | ||
| } | ||
| Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), | ||
| }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| let envd_request = target_port == proxy::effective_envd_port(&metadata); | ||
| let envd_authorized = envd_request |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
32bf340 to
0938eed
Compare
|
🔍 OpenCodeReview found 11 issue(s) in this PR.
|
| let parent = managed_parent(path)?; | ||
| let file = match open_secret(path) { | ||
| Ok(file) => file, | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), | ||
| Err(error) => { | ||
| return Err(error).with_context(|| format!("open managed secret {}", path.display())); | ||
| } | ||
| }; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; | ||
| read_file(path, file, max_len).map(Some) |
There was a problem hiding this comment.
read opens the secret before validating its parent, and validate_directory plus read_file are separate pathname-based checks. Since ancestor components can be symlinks or be replaced concurrently, the opened file is not guaranteed to reside in the directory that was validated; the same issue also affects the Existing branch in create. Bind the open and validation to the same directory/file descriptors and reject symlinked ancestors.
| let parent = managed_parent(path)?; | ||
| create_directory(parent)?; | ||
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; | ||
|
|
||
| let mut temporary = tempfile::NamedTempFile::new_in(parent) | ||
| .with_context(|| format!("create temporary secret in {}", parent.display()))?; | ||
| set_permissions(temporary.path(), 0o600)?; | ||
| temporary | ||
| .write_all(contents) | ||
| .with_context(|| format!("write temporary secret in {}", parent.display()))?; | ||
| temporary | ||
| .as_file() | ||
| .sync_all() | ||
| .with_context(|| format!("sync temporary secret in {}", parent.display()))?; | ||
|
|
||
| match temporary.persist_noclobber(path) { | ||
| Ok(_) => { | ||
| File::open(parent) | ||
| .and_then(|directory| directory.sync_all()) | ||
| .with_context(|| format!("sync managed secret directory {}", parent.display()))?; | ||
| Ok(CreateOutcome::Created) | ||
| } | ||
| Err(error) if error.error.kind() == io::ErrorKind::AlreadyExists => { | ||
| validate_directory(parent).with_context(|| { | ||
| format!("validate managed secret directory {}", parent.display()) | ||
| })?; | ||
| open_secret(path) | ||
| .map(CreateOutcome::Existing) | ||
| .with_context(|| format!("open managed secret {}", path.display())) | ||
| } | ||
| Err(error) => { | ||
| Err(error.error).with_context(|| format!("persist managed secret {}", path.display())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The directory is validated by pathname, but all subsequent operations use fresh pathname lookups. Ancestors of parent are still followed (only the final secret component uses O_NOFOLLOW), and create_directory, set_permissions, temporary-file creation, persist_noclobber, and the later open_secret are not bound to the validated directory inode. A replaceable/symlinked ancestor, or a directory swap between these calls, can therefore redirect secret creation/readback or permission changes into an unintended secrets directory. Perform the validation and file operations through an opened directory descriptor (for example, openat/renameat with no-follow protections), or otherwise establish an equivalent inode-bound guarantee.
0938eed to
a39a344
Compare
| --context|--kubeconfig) | ||
| if ((i + 1 >= ${#ARGS[@]})); then | ||
| echo "${arg} requires a value" >&2 | ||
| exit 1 | ||
| fi | ||
| KUBECTL_TARGET_ARGS+=("${arg}" "${ARGS[i + 1]}") | ||
| i=$((i + 1)) | ||
| ;; | ||
| --context=*|--kubeconfig=*) KUBECTL_TARGET_ARGS+=("${arg}") ;; |
There was a problem hiding this comment.
Reject -n/--namespace arguments here and require callers to use K8S_NAMESPACE. These flags are still forwarded to the final kubectl apply, while rendering, Secret bootstrapping, and rollout restart all use K8S_NAMESPACE. A caller passing another namespace can therefore create the namespace and Secret in one location and then have apply fail on the rendered namespace mismatch (or target resources elsewhere), leaving a partial installation.
| --dry-run=client|--dry-run=server) DRY_RUN=1 ;; | ||
| --dry-run=none) DRY_RUN=0 ;; |
There was a problem hiding this comment.
Reject every unrecognized --dry-run=* value before calling ensure_namespace. Currently a value outside this allowlist leaves DRY_RUN=0, so the script can create the namespace and agentenv-auth Secret before the final kubectl apply rejects the dry-run argument. This is especially risky with kubectl-version-specific/legacy spellings because an operation requested as a dry run can mutate the live cluster. Parse the value explicitly and fail closed unless it is known to mean either dry-run or none.
Suggestion:
| --dry-run=client|--dry-run=server) DRY_RUN=1 ;; | |
| --dry-run=none) DRY_RUN=0 ;; | |
| --dry-run=client|--dry-run=server) DRY_RUN=1 ;; | |
| --dry-run=none) DRY_RUN=0 ;; | |
| --dry-run=*) | |
| echo "unsupported dry-run value: ${arg#--dry-run=}" >&2 | |
| exit 1 | |
| ;; |
| -H "X-API-Key: ${AENV_API_KEY}" \ | ||
| -H "x-agentenv-sandbox-id: ${sandbox_id}" \ | ||
| -H "x-agentenv-target-port: 49983" \ | ||
| -H "x-agentenv-target-port: ${AENV_ENVD_PORT}" \ |
There was a problem hiding this comment.
AENV_ENVD_PORT is only given a fixed 49983 default in the shared E2E helpers; it is not derived from tools.control_plane_port or from the deployed sandbox configuration. When the control-plane port is customized (which this change explicitly claims to support), every proxy request here targets the wrong port and the suite fails or can probe another service. Please obtain the effective configured port during runtime setup (or use the API/config value) rather than relying on this hardcoded default.
Suggestion:
| -H "x-agentenv-target-port: ${AENV_ENVD_PORT}" \ | |
| -H "x-agentenv-target-port: ${EFFECTIVE_ENVD_PORT}" \ |
| while IFS= read -r node_url; do | ||
| [[ -z "${node_url}" ]] && continue | ||
| api_get_no_auth_at "${node_url}" "/metrics" | ||
| assert_status "$HTTP_STATUS" "200" "node /metrics works without auth at ${node_url}" | ||
| done < <(printf '%s\n' "${AENV_NODE_URLS:-}" | tr ' ' '\n') |
There was a problem hiding this comment.
In clustered mode, this loop is the only assertion that node-local /metrics endpoints are reachable, but it is skipped entirely when AENV_NODE_URLS is empty. The preceding gateway check only verifies that the client listener returns 404, so a misconfigured clustered environment with no discovered node URLs can pass this suite without testing any node metrics endpoint. Assert that clustered mode has at least one node URL (or fail during runtime setup) before entering the loop.
Suggestion:
| while IFS= read -r node_url; do | |
| [[ -z "${node_url}" ]] && continue | |
| api_get_no_auth_at "${node_url}" "/metrics" | |
| assert_status "$HTTP_STATUS" "200" "node /metrics works without auth at ${node_url}" | |
| done < <(printf '%s\n' "${AENV_NODE_URLS:-}" | tr ' ' '\n') | |
| if e2e_mode_is_clustered; then | |
| assert_not_empty "${AENV_NODE_URLS:-}" "clustered mode exposes at least one node endpoint" | |
| fi | |
| while IFS= read -r node_url; do | |
| [[ -z "${node_url}" ]] && continue | |
| api_get_no_auth_at "${node_url}" "/metrics" | |
| assert_status "${HTTP_STATUS}" "200" "node /metrics works without auth at ${node_url}" | |
| done < <(printf '%s\n' "${AENV_NODE_URLS:-}" | tr ' ' '\n') |
| request.headers_mut().remove(ENVD_ACCESS_TOKEN_HEADER); | ||
| return proxy::sandbox_not_found_response(sandbox_id); | ||
| } | ||
| Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), |
There was a problem hiding this comment.
This drops the metadata-store/orchestrator error without recording it. Because this lookup now sits on every resolved proxy request, a store outage will produce opaque 500 responses with no cause in the logs, unlike resolve_proxy_request, which logs lookup failures with the sandbox ID and error. Preserve the error and emit a contextual warning before returning 500.
Suggestion:
| Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), | |
| Err(err) => { | |
| tracing::warn!(sandbox_id = %sandbox_id, error = %err, "failed to load sandbox metadata for proxy authorization"); | |
| return StatusCode::INTERNAL_SERVER_ERROR.into_response(); | |
| } |
| let mut headers = route.to_vec(); | ||
| headers.push((ENVD_ACCESS_TOKEN_HEADER, envd_token.expose())); | ||
| assert_ne!( | ||
| get_status(&app, "/proxy/health", &headers).await, | ||
| StatusCode::UNAUTHORIZED | ||
| ); |
There was a problem hiding this comment.
This assertion does not prove that the valid envd token authenticated the request or that the envd endpoint was reached: any non-401 response, including a 404, 500, or proxy routing error, passes. Use a deterministic successful envd fixture/response and assert its expected status (and ideally response body), and apply the same strengthening to the earlier non-secure assert_ne! check in this test.
| builder.recursive(true); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::DirBuilderExt; | ||
| builder.mode(0o700); | ||
| } | ||
| builder | ||
| .create(path) | ||
| .with_context(|| format!("create managed secret directory {}", path.display())) |
There was a problem hiding this comment.
recursive(true) creates or traverses every ancestor of the secrets directory before any ancestor is checked for symlinks, ownership, or permissions. If an attacker can influence or replace a component of the configured home path, this can resolve secrets into an attacker-chosen directory; the later validation only covers that resolved final directory and cannot establish that it is under the intended root. Create the directory hierarchy one component at a time with no-follow semantics, or require the full parent hierarchy to be pre-created and validate it before writing the secret.
| self.matches_for( | ||
| format!("{TRAFFIC_ACCESS_TOKEN_PREFIX}-{subject}").as_bytes(), | ||
| candidate, | ||
| ) |
There was a problem hiding this comment.
matches_traffic is used on the per-request proxy authentication path, so this format! allocates a new String for every private-ingress request before computing the HMAC. This adds avoidable allocation and formatting overhead to a hot path; construct the fixed prefix plus the canonical sandbox ID in a stack-backed buffer (or expose a non-allocating canonical subject helper) and pass that byte slice to matches_for.
| --context|--kubeconfig) | ||
| if ((i + 1 >= ${#ARGS[@]})); then | ||
| echo "${arg} requires a value" >&2 | ||
| exit 1 | ||
| fi | ||
| KUBECTL_TARGET_ARGS+=("${arg}" "${ARGS[i + 1]}") | ||
| i=$((i + 1)) | ||
| ;; | ||
| --context=*|--kubeconfig=*) KUBECTL_TARGET_ARGS+=("${arg}") ;; |
There was a problem hiding this comment.
Only --context and --kubeconfig are forwarded to Secret bootstrap, namespace creation, and rollout restart, while the main apply receives all of "$@". Valid kubectl global target/auth flags such as --server, --cluster, --user, --token, --as, and TLS options can therefore make these operations address a different cluster or identity than the apply. This can copy a credential between clusters or mutate/restart resources in the wrong cluster. Ensure every kubectl invocation uses the same complete set of global flags, ideally by separating supported global flags from apply-specific flags once and rejecting unsupported ambiguous arguments.
| elif [[ "${DRY_RUN}" == "1" ]]; then | ||
| API_KEY_VALUE="$(generate_api_key)" |
There was a problem hiding this comment.
Treating --dry-run=server like client dry-run skips namespace creation. On a fresh cluster, server-side dry-run does not persist the Namespace object before validating subsequent namespaced resources, so those resources can fail with namespaces "..." not found even though a real first apply succeeds. Handle server dry-run separately, for example by requiring/prechecking an existing namespace or documenting and reporting this limitation explicitly.
| else | ||
| ensure_namespace || exit 1 | ||
| bootstrap_api_key || exit 1 | ||
| fi |
There was a problem hiding this comment.
These calls persist the namespace and API-key Secret before kubectl apply parses and validates the full argument list. For example, an invalid apply option causes the script to fail later while leaving credentials and a namespace behind. Validate the apply command/options before bootstrapping, or restructure the flow so persistent bootstrap changes occur only once argument validation has succeeded (and add a failure-path test).
| request.headers_mut().remove(ENVD_ACCESS_TOKEN_HEADER); | ||
| return proxy::sandbox_not_found_response(sandbox_id); | ||
| } | ||
| Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), |
There was a problem hiding this comment.
The orchestrator failure is discarded here and converted to a bare 500 response. This removes the sandbox ID and underlying error context, making backend outages indistinguishable from middleware defects and difficult to diagnose in production. Please log the error with the sandbox ID (without credentials) or propagate it through the repository's standard error handling path.
Suggestion:
| Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), | |
| Err(err) => { | |
| tracing::error!(sandbox_id = %sandbox_id, error = %err, "failed to load sandbox metadata for proxy authorization"); | |
| return StatusCode::INTERNAL_SERVER_ERROR.into_response(); | |
| } |
| Some(( | ||
| parse_sandbox_id_header(request.headers()).ok()?, | ||
| parse_target_port_header(request.headers()).ok()?, | ||
| )) |
There was a problem hiding this comment.
This fallback makes authentication treat any request classified as a sandbox proxy request as header-routed, even when it did not use an explicit /proxy path or a valid host route. In particular, an unmatched control-plane URI carrying x-sandbox-id/e2b-sandbox-id and x-agentenv-target-port is classified by is_sandbox_proxy_request via the MatchedPath fallback and can then be authorized with only the sandbox traffic token. That allows typos or unknown control-plane paths to be dispatched to an arbitrary sandbox instead of being rejected by control-plane API-key authentication. Restrict this fallback to the actual proxy fallback entry point (or require an explicit proxy prefix/validated host route) rather than deriving the route solely from attacker-controlled headers.
| let file = match open_secret(path) { | ||
| Ok(file) => file, | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), | ||
| Err(error) => { | ||
| return Err(error).with_context(|| format!("open managed secret {}", path.display())); | ||
| } | ||
| }; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; |
There was a problem hiding this comment.
The file and parent directory are resolved by separate pathname operations, and the file is opened before the parent is validated. If a writable ancestor is renamed or replaced between these calls, file can come from one directory while validate_directory checks another; O_NOFOLLOW protects only the final file component. Since this content becomes an API key or sandbox-token seed, anchor the operation to a no-follow directory descriptor and open the child relative to it (openat/openat2 or a capability-directory API), then validate the descriptor metadata.
| create_directory(parent)?; | ||
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; | ||
|
|
||
| let mut temporary = tempfile::NamedTempFile::new_in(parent) | ||
| .with_context(|| format!("create temporary secret in {}", parent.display()))?; |
There was a problem hiding this comment.
This check-then-act sequence repeatedly resolves parent by pathname. With a writable/configurable ancestor, the validated directory can be replaced before set_permissions or NamedTempFile::new_in; set_permissions also follows a replacement symlink, so the daemon may chmod an unintended object, and later persistence/sync can target a different directory. Open the dedicated directory once with no-follow directory semantics, validate and chmod via that descriptor, and perform temporary-file creation, rename, and sync relative to the same descriptor.
| KUBECTL_TARGET_ARGS=() | ||
| DRY_RUN=0 | ||
| ARGS=("$@") | ||
| for ((i = 0; i < ${#ARGS[@]}; i++)); do |
There was a problem hiding this comment.
The auxiliary commands use KUBECTL_TARGET_ARGS, which preserves only --context and --kubeconfig, while the actual apply still receives the complete "$@". If a caller selects the cluster with other kubectl connection/authentication flags such as --server, --token, --user, or --certificate-authority, namespace creation, Secret reads/creation, and rollout restart can run against the default cluster (or fail) before/after the manifest is applied to the intended cluster. Preserve and reuse the complete kubectl target configuration for these commands, while still preventing runner-specific arguments from being duplicated.
| OVERLAY_PATH="${TEMP_DIR}/k8s/overlays/${OVERLAY_NAME}" | ||
| if [[ ! -d "${OVERLAY_PATH}" ]]; then | ||
| echo "unknown overlay: ${OVERLAY_NAME}" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
K8S_OVERLAY is used directly to construct a path, so values containing .. can escape ${TEMP_DIR}/k8s/overlays after the existence check (for example, ../base or a deeper traversal). The subsequent sed operations and kubectl apply -k can then operate on an unintended directory. Validate that the overlay is a simple expected name, or resolve and verify that the resulting path remains below the overlays directory.
| "${KUBECTL_BIN}" "${KUBECTL_TARGET_ARGS[@]}" -n "${NAMESPACE}" create secret generic agentenv-auth \ | ||
| --from-file="AENV_API_KEY=${secret_file}" >/dev/null 2>"${create_error}" || true | ||
|
|
||
| if ! API_KEY_VALUE="$(read_existing_api_key)"; then |
There was a problem hiding this comment.
When agentenv-auth already exists but has no AENV_API_KEY, read_existing_api_key returns empty and create secret fails with AlreadyExists; the error is ignored, the reread remains empty, and the script exits. This makes a partially created or manually provisioned Secret unrecoverable through the normal apply path. Handle the existing-empty case by updating the Secret (or fail with an explicit remediation path) instead of attempting create-only bootstrap.
| if [[ "${MODE}" != "delete" ]]; then | ||
| restore_xtrace=0 | ||
| if [[ $- == *x* ]]; then | ||
| restore_xtrace=1 | ||
| set +x | ||
| fi |
There was a problem hiding this comment.
The added shell branches have no focused coverage visible here for invalid namespace/overlay input, forwarding non-context kubectl connection flags, render redaction, dry-run variants, xtrace suppression, or existing/empty Secret behavior. These paths control credentials and cluster targeting, so a small mocked-kubectl test matrix would catch regressions that the end-to-end happy path is unlikely to expose.
| if [[ "${_E2E_API_KEY_FROM_USER}" != "1" ]]; then | ||
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | ||
| die "Failed to read the Compose deployment API key" | ||
| fi |
There was a problem hiding this comment.
When shell tracing is enabled (for example, bash -x in CI diagnostics), Bash prints the result of this assignment, exposing the generated deployment API key in logs. The analogous Kubernetes assignment below has the same issue, and later traced curl -H commands can expose it again. Disable and restore xtrace around secret acquisition/use (as deploy/k8s/run.sh already does), or route authenticated commands through a helper that suppresses tracing.
Suggestion:
| if [[ "${_E2E_API_KEY_FROM_USER}" != "1" ]]; then | |
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | |
| die "Failed to read the Compose deployment API key" | |
| fi | |
| if [[ "${_E2E_API_KEY_FROM_USER}" != "1" ]]; then | |
| local restore_xtrace=0 | |
| if [[ $- == *x* ]]; then | |
| restore_xtrace=1 | |
| set +x | |
| fi | |
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | |
| die "Failed to read the Compose deployment API key" | |
| [[ "${restore_xtrace}" == "0" ]] || set -x | |
| fi |
| _key: &str, | ||
| ) -> Option<Self::Claims> { | ||
| let admin_token = non_empty_header(headers, "X-Admin-Token"); | ||
| if key == "X-Admin-Token" { | ||
| return admin_token.then_some(Claims); | ||
| } | ||
|
|
||
| if non_empty_header(headers, "X-API-Key") | ||
| || non_empty_header(headers, "X-Team-ID") | ||
| || admin_token | ||
| { | ||
| Some(Claims) | ||
| } else { | ||
| None | ||
| } | ||
| self.has_valid_api_key(headers).then_some(Claims) |
There was a problem hiding this comment.
This adapter now ignores the generated route's requested header and auth scheme and returns claims whenever x-api-key is valid. The generated OpenAPI routes use this method for X-Admin-Token and X-Team-ID, and use the Basic adapter for Bearer auth; consequently, a caller with the general API key can satisfy admin/team/Bearer security requirements without supplying the credential that the route declares. The outer middleware also only checks the same general API key, so it does not restore the distinction. Preserve scheme-specific validation in these adapters, or ensure the centralized middleware enforces the route's matched security scheme before bypassing the generated checks.
Suggestion:
| _key: &str, | |
| ) -> Option<Self::Claims> { | |
| let admin_token = non_empty_header(headers, "X-Admin-Token"); | |
| if key == "X-Admin-Token" { | |
| return admin_token.then_some(Claims); | |
| } | |
| if non_empty_header(headers, "X-API-Key") | |
| || non_empty_header(headers, "X-Team-ID") | |
| || admin_token | |
| { | |
| Some(Claims) | |
| } else { | |
| None | |
| } | |
| self.has_valid_api_key(headers).then_some(Claims) | |
| key: &str, | |
| ) -> Option<Self::Claims> { | |
| (key.eq_ignore_ascii_case(API_KEY_HEADER) | |
| && self.has_valid_api_key(headers)) | |
| .then_some(Claims) |
| let file = match open_secret(path) { | ||
| Ok(file) => file, | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), | ||
| Err(error) => { | ||
| return Err(error).with_context(|| format!("open managed secret {}", path.display())); | ||
| } | ||
| }; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; |
There was a problem hiding this comment.
open_secret(path) resolves the full path before validate_directory(parent) checks the parent, and the check is path-based. If the parent or an ancestor can be renamed/replaced concurrently, the opened descriptor may refer to a file in an unvalidated directory; this can disclose an attacker-selected 0600 file or bypass the intended directory boundary. Open and validate the parent through stable directory handles (and use descriptor-relative file operations), rather than validating a pathname after resolution.
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; | ||
|
|
||
| let mut temporary = tempfile::NamedTempFile::new_in(parent) |
There was a problem hiding this comment.
The directory is validated and then repeatedly resolved by path for creation and persistence. Between these operations, a process that can rename/replace parent or an ancestor can redirect set_permissions, NamedTempFile::new_in, and persist_noclobber to a different directory, potentially writing the secret outside the validated 0700 directory (and the final directory sync has the same path-race). Use directory/file-descriptor-relative operations (or otherwise hold a stable directory handle and verify the resolved inode before each operation), and apply the same protection to all ancestor components that can be replaced.
| if 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" | ||
| "using a node-local managed sandbox access-token seed; configure AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED with the same value on every node in a clustered deployment" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This warning is now emitted for every clustered configuration, including cases where access_token_hash_seed is explicitly configured and therefore the node-local managed seed is not used. The message claims a node-local seed is being used, so it is factually incorrect for the explicit shared-seed configuration and can mislead operators. Keep the original guard (or base the warning on the resolved seed source).
| if 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" | ||
| "using a node-local managed sandbox access-token seed; configure AENV_SANDBOX_ACCESS_TOKEN_HASH_SEED with the same value on every node in a clustered deployment" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Correction: this warning is reached only after the explicit access_token_hash_seed branch returns, so the clustered warning is correctly limited to the node-local managed-seed case. No change is needed for this block.
| OVERLAY_PATH="${TEMP_DIR}/k8s/overlays/${OVERLAY_NAME}" | ||
| if [[ ! -d "${OVERLAY_PATH}" ]]; then |
There was a problem hiding this comment.
K8S_OVERLAY is interpolated into a filesystem path and only checked for directory existence. Values such as ../base or ../../... can escape the copied overlays directory, causing subsequent sed and kubectl ... -k operations to use an unintended directory. Since the repository has a fixed set of overlays, validate OVERLAY_NAME against an allowlist (for example default|local-dev) or reject path separators and .. before constructing the path.
| "${KUBECTL_BIN}" "${KUBECTL_TARGET_ARGS[@]}" -n "${NAMESPACE}" create secret generic agentenv-auth \ | ||
| --from-file="AENV_API_KEY=${secret_file}" >/dev/null 2>"${create_error}" || true |
There was a problem hiding this comment.
This imperative Secret is not the Secret consumed by the workloads after kustomization. base/kustomization.yaml defines agentenv-auth via secretGenerator without disableNameSuffixHash, so kubectl apply -k creates a hashed name (and rewrites the Deployment/DaemonSet references to that hashed name), while this command creates the un-hashed agentenv-auth. The bootstrap key is therefore unused; each apply can generate a different key and the script's persisted-key reuse does not guarantee the key loaded by the pods, causing authentication failures. Either disable the generator name hash and use this Secret consistently, or remove imperative creation and inject the persisted key into the generator before apply.
| fi | ||
| } | ||
|
|
||
| if [[ "${MODE}" != "delete" ]]; then |
There was a problem hiding this comment.
delete skips API-key rendering, so the copied kustomization retains the empty secretGenerator literal. Kustomize names generated Secrets with a content hash; therefore delete -k targets the hash for an empty key rather than the hash of the real key applied earlier, and the deployed generated Secret is left behind. The imperative bootstrap Secret agentenv-auth is also not part of the kustomization and is never deleted. Delete should resolve/use the persisted key (or otherwise explicitly delete the generated and bootstrap Secret resources) before invoking kustomize delete.
| apply) | ||
| "${KUBECTL_BIN}" apply -k "${OVERLAY_PATH}" "$@" |
There was a problem hiding this comment.
The script parses --context and --kubeconfig into KUBECTL_TARGET_ARGS, but the actual render/apply/delete operations do not use that array. As a result, K8S_NAMESPACE/Secret bootstrap and rollout may target the requested cluster while kubectl kustomize/apply -k/delete -k run against the default kubectl context (and kustomize may receive these flags inconsistently via "$@"). This can deploy or delete resources in the wrong cluster. Add "${KUBECTL_TARGET_ARGS[@]}" to the relevant kubectl invocations, while keeping target flags out of the kustomize-specific argument list.
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | ||
| die "Failed to read the Compose deployment API key" |
There was a problem hiding this comment.
This expands the generated API key directly in a shell assignment. If the E2E runner or CI invokes the suite with set -x, Bash will write the expanded assignment—and therefore the secret—to the trace log. The Kubernetes assignment below has the same exposure. Temporarily disable xtrace while reading and assigning deployment-generated keys (restoring it afterward), as deploy/k8s/run.sh already does around secret handling.
Suggestion:
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | |
| die "Failed to read the Compose deployment API key" | |
| local restore_xtrace=0 | |
| if [[ $- == *x* ]]; then | |
| restore_xtrace=1 | |
| set +x | |
| fi | |
| AENV_API_KEY="$(_compose_cmd exec -T agentenv-a cat /workspace/env/secrets/api-key)" || | |
| die "Failed to read the Compose deployment API key" | |
| [[ "${restore_xtrace}" == "0" ]] || set -x |
| Ok(contents) | ||
| } | ||
|
|
||
| pub(crate) fn create(path: &Path, contents: &[u8]) -> Result<CreateOutcome> { |
There was a problem hiding this comment.
create accepts arbitrarily large contents, but every current reader supplies a finite max_len and rejects files above it. This allows the abstraction to successfully persist a secret that it cannot later read, and permits an unexpectedly large temporary write if this helper is later fed untrusted data. Pass the policy limit into create and reject contents.len() before creating or writing the temporary file.
| let mut temporary = tempfile::NamedTempFile::new_in(parent) | ||
| .with_context(|| format!("create temporary secret in {}", parent.display()))?; | ||
| set_permissions(temporary.path(), 0o600)?; | ||
| temporary | ||
| .write_all(contents) | ||
| .with_context(|| format!("write temporary secret in {}", parent.display()))?; | ||
| temporary | ||
| .as_file() | ||
| .sync_all() | ||
| .with_context(|| format!("sync temporary secret in {}", parent.display()))?; | ||
|
|
||
| match temporary.persist_noclobber(path) { |
There was a problem hiding this comment.
The checks on parent and the subsequent NamedTempFile/persist_noclobber operations all resolve the directory by pathname. If another local principal can rename or replace secrets after these checks, the temporary file, permission change, or final persistence can target a different directory, bypassing the ownership/mode guarantees and potentially placing secret material elsewhere. Keep an opened directory descriptor and perform the creation/rename descriptor-relatively (with no-follow semantics), or otherwise synchronize/protect the directory for the entire operation.
| open_secret(path) | ||
| .map(CreateOutcome::Existing) | ||
| .with_context(|| format!("open managed secret {}", path.display())) |
There was a problem hiding this comment.
open_secret uses O_NOFOLLOW, but that only protects the final path component; parent components are still resolved normally. In this CreateOutcome::Existing path, the secrets directory is validated and then the file is opened in a separate operation, so a concurrent replacement of the parent directory with a symlink can make the helper open an attacker-controlled file before read_file validates only the file metadata. Since this material is used to forge sandbox authentication tokens, use directory-FD-relative operations (for example, openat/openat2 with no-follow constraints) or otherwise make parent validation and file opening a single race-safe operation.
| fn managed_parent(path: &Path) -> Result<&Path> { | ||
| let parent = path.parent().context("managed secret path has no parent")?; | ||
| if parent.file_name().is_none_or(|name| name != "secrets") { |
There was a problem hiding this comment.
Checking only the final component named secrets does not prevent symlink traversal through an ancestor of parent. create_directory(..., recursive = true), set_permissions, and new_in can therefore follow a mutable ancestor symlink and operate outside the intended managed-secret tree. Validate every ancestor without following symlinks, or use descriptor-relative no-follow directory operations when constructing and accessing the path.
| pub fn generate_traffic(&self, subject: SandboxId) -> String { | ||
| self.generate_for(format!("{TRAFFIC_ACCESS_TOKEN_PREFIX}-{subject}").as_bytes()) | ||
| } |
There was a problem hiding this comment.
Traffic-token authentication can run on every private application proxy request, but each call allocates a new String via format! solely to build the HMAC subject, and matches_traffic repeats the same allocation on validation. This adds avoidable per-request heap work and leaves the domain-separation format untyped; consider a byte-oriented subject builder (or a fixed stack buffer) shared by generation and validation.
| metadata | ||
| .paused_state | ||
| .as_ref() | ||
| .and_then(|state| state.control_plane_port()) | ||
| .unwrap_or_else(|| ConfigManager::global_config().tools.control_plane_port) |
There was a problem hiding this comment.
This is a temporary workaround to get the actual control plane port. We should record it in SandboxMetadata in the future.
| cat "${file}" | ||
| } | awk ' | ||
| NR == 1 { api_key = $0; next } | ||
| /^ - AENV_API_KEY=/ { print " - AENV_API_KEY=" api_key; replaced = 1; next } |
There was a problem hiding this comment.
The renderer only replaces a line with exactly six spaces followed by - AENV_API_KEY=. A harmless formatting change, a YAML block/literal representation, or another generator layout will leave the empty key in the rendered kustomization; kubectl kustomize then emits an empty Secret and the failure is discovered only at workload startup. Replace the generator value structurally (or validate the expected entry before rendering) instead of depending on this exact indentation and text prefix.
Suggestion:
| /^ - AENV_API_KEY=/ { print " - AENV_API_KEY=" api_key; replaced = 1; next } | |
| /^[[:space:]]*-[[:space:]]*AENV_API_KEY=/ { print " - AENV_API_KEY=" api_key; replaced = 1; next } |
| "${KUBECTL_BIN}" "${KUBECTL_TARGET_ARGS[@]}" -n "${NAMESPACE}" rollout restart \ | ||
| deployment/agentenv-gateway daemonset/agentenv-node |
There was a problem hiding this comment.
kubectl apply still receives the caller's raw "$@", so a caller can pass -n/--namespace and direct the apply request at a namespace different from ${NAMESPACE}. The Secret bootstrap and the subsequent rollout restart are explicitly performed in ${NAMESPACE}, so this invocation can apply workloads that reference agentenv-auth in another namespace while the Secret exists only in ${NAMESPACE}; the command can then restart unrelated workloads or fail to restart the applied ones. Reject namespace flags or normalize/remove them and pass the validated ${NAMESPACE} consistently to apply, delete, and rollout operations.
Suggestion:
| "${KUBECTL_BIN}" "${KUBECTL_TARGET_ARGS[@]}" -n "${NAMESPACE}" rollout restart \ | |
| deployment/agentenv-gateway daemonset/agentenv-node | |
| "${KUBECTL_BIN}" "${KUBECTL_TARGET_ARGS[@]}" apply -k "${OVERLAY_PATH}" -n "${NAMESPACE}" |
| : "${AENV_API_KEY:=e2b_0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef}" | ||
| : "${AENV_TEMPLATE_ID:=ubuntu}" | ||
| : "${AENV_PROXY_URL:=${AENV_URL}/proxy}" | ||
| : "${AENV_ENVD_PORT:=49983}" |
There was a problem hiding this comment.
AENV_ENVD_PORT is defaulted independently to 49983, while the server's actual control-plane port comes from tools.control_plane_port in the selected config. When E2E runs with a config that changes that port (the runner explicitly supports CONFIG_PATH), all proxy/auth/metrics requests use the wrong target port and fail or may hit an unrelated service. Please derive this value from the active config/runtime, or require it to be supplied by the runtime setup instead of hard-coding the default here.
Suggestion:
| : "${AENV_ENVD_PORT:=49983}" | |
| : "${AENV_ENVD_PORT:=${AENV_CONTROL_PLANE_PORT:-49983}}" |
| export E2B_API_URL="${AENV_URL}" | ||
| export E2B_SANDBOX_URL="${AENV_PROXY_URL}" | ||
| export E2B_API_KEY="e2b_000000" | ||
| export E2B_API_KEY="${AENV_API_KEY}" |
There was a problem hiding this comment.
This assignment is currently unreachable because the suite exits at line 17 before reaching it. As a result, this change does not affect the code-interpreter compatibility test until the temporary skip is removed; keep the credential update aligned with re-enabling the suite (or remove the dead block while it remains disabled).
| #[derive(Clone)] | ||
| pub struct ApiKey(String); |
There was a problem hiding this comment.
ApiKey is a cloneable wrapper around the credential-bearing String, and ApiImpl derives Clone while storing this value directly. Cloning router/state instances can therefore create multiple long-lived heap copies of the API key that cannot be reliably cleared, increasing exposure in memory dumps or crash diagnostics. Keep a single shared key allocation (for example, store it behind Arc in the cloneable service state) and avoid deriving Clone for the secret itself, or use a zeroizing secret representation if the ownership model permits.
| Ok(SandboxNetworkPolicy::new( | ||
| true, | ||
| base_policy_from_allow_internet_access(body.allow_internet_access), | ||
| policy, | ||
| )) |
There was a problem hiding this comment.
This update path always reconstructs the policy with allow_public_traffic = true. Since replace_sandbox_network_policy replaces the stored policy, any network PUT on a sandbox created with private ingress (allowPublicTraffic: false) makes its proxy URLs publicly accessible, bypassing the traffic-token requirement. Preserve the existing ingress setting when applying an update, or include and validate the ingress field in the update model instead of forcing it to true.
Suggestion:
| Ok(SandboxNetworkPolicy::new( | |
| true, | |
| base_policy_from_allow_internet_access(body.allow_internet_access), | |
| policy, | |
| )) | |
| Ok(SandboxNetworkPolicy::new( | |
| /* preserve the existing allow_public_traffic value at the replacement call site */ | |
| true, | |
| base_policy_from_allow_internet_access(body.allow_internet_access), | |
| policy, | |
| )) |
| Ok(SandboxNetworkPolicy::new( | ||
| true, | ||
| base_policy_from_allow_internet_access(body.allow_internet_access), | ||
| policy, | ||
| )) |
There was a problem hiding this comment.
Correction to the preceding review comment: no issue here. replace_sandbox_network_policy_inner explicitly overwrites this value with the sandbox's existing allow_public_traffic setting before applying and persisting the policy, so a network update does not make private ingress public. Please disregard the previous comment.
| let file = match open_secret(path) { | ||
| Ok(file) => file, | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), |
There was a problem hiding this comment.
When the secret file is absent, this returns Ok(None) before validating parent. A symlinked, foreign-owned, or world-accessible secrets directory therefore gets treated as a normal missing-secret case; the caller then proceeds to create the secret through that untrusted directory. Validate the parent first, while preserving NotFound only for a genuinely absent directory/file.
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; |
There was a problem hiding this comment.
The security checks and subsequent mutations all reopen the parent by pathname. Between validate_directory_identity, set_permissions, persist_noclobber, and the directory sync, an attacker/process able to rename or replace an ancestor can make these operations target a different directory than the one validated. Keep a stable directory handle and perform the creation/permission checks relative to it (or otherwise serialize and eliminate this pathname TOCTOU).
| fn create_directory(path: &Path) -> Result<()> { | ||
| let mut builder = fs::DirBuilder::new(); | ||
| builder.recursive(true); |
There was a problem hiding this comment.
create_directory uses recursive, pathname-based creation, so an existing ancestor of secrets may be a symlink and will be traversed before the final secrets component is validated. Since home_path is configuration-derived, this can redirect the managed-secret directory (and the generated API key/seed) outside the intended data directory. Creation should walk/validate every ancestor without following symlinks (or use directory-fd/openat-style operations) rather than relying only on the final parent check.
| KUBECTL_TARGET_ARGS=() | ||
| DRY_RUN=0 | ||
| ARGS=("$@") | ||
| for ((i = 0; i < ${#ARGS[@]}; i++)); do |
There was a problem hiding this comment.
The bootstrap and post-apply operations use only KUBECTL_TARGET_ARGS, while the actual apply/delete/render invocation forwards the complete "$@". Consequently, callers passing kubectl global options such as --server, --cluster, --user, --token, --as, or TLS/impersonation flags can make Secret creation/read and rollout restart target a different cluster or identity than the apply. Forward the complete supported kubectl target argument set consistently to every kubectl command, or reject unsupported global options instead of silently dropping them.
| if [[ -n "${API_KEY_VALUE}" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| secret_file="${TEMP_DIR}/bootstrap-api-key" |
There was a problem hiding this comment.
If agentenv-auth already exists with an empty or missing AENV_API_KEY, this path attempts create secret, which necessarily fails with AlreadyExists, then fails again after rereading the still-empty value. The deployment cannot recover without manual Secret deletion/editing, and the resulting error is misleading. Detect an existing malformed Secret explicitly and report that it must be repaired (or update it only when an explicit AENV_API_KEY is supplied), rather than treating it as a missing Secret and attempting an unconditional create.
| if [[ "${AENV_API_KEY+x}" == "x" ]]; then | ||
| if [[ -z "${AENV_API_KEY}" ]]; then | ||
| echo "AENV_API_KEY must not be empty" >&2 | ||
| return 1 | ||
| fi | ||
| _E2E_API_KEY_FROM_USER=1 | ||
| fi |
There was a problem hiding this comment.
Validate a user-provided key against the same 32-256 URL-safe-character rule here, before starting the deployment. As written, a non-empty but invalid key is passed into Compose/Kubernetes; the server rejects it during startup, so the later validation is never reached and the test waits for the full health/rollout timeout before reporting an unrelated readiness failure. Reusing a small validation helper here and after reading generated secrets would fail immediately with the actual configuration error.
Suggestion:
| if [[ "${AENV_API_KEY+x}" == "x" ]]; then | |
| if [[ -z "${AENV_API_KEY}" ]]; then | |
| echo "AENV_API_KEY must not be empty" >&2 | |
| return 1 | |
| fi | |
| _E2E_API_KEY_FROM_USER=1 | |
| fi | |
| if [[ "${AENV_API_KEY+x}" == "x" ]]; then | |
| if [[ ! "${AENV_API_KEY}" =~ ^[A-Za-z0-9._~-]{32,256}$ ]]; then | |
| echo "AENV_API_KEY must contain between 32 and 256 URL-safe characters" >&2 | |
| return 1 | |
| fi | |
| _E2E_API_KEY_FROM_USER=1 | |
| fi |
| suite_summary "08_auth" || true | ||
| exit 1 | ||
| fi | ||
| wait_for_sandbox_state "$secure_sandbox_id" "running" 30 |
There was a problem hiding this comment.
wait_for_sandbox_state returns nonzero when provisioning times out, but this call ignores that status and continues with proxy authentication assertions. A sandbox that never reaches running can therefore be reported as a series of authentication failures instead of a readiness failure. Gate the dependent assertions (and do the same for the second sandbox) on a successful wait.
Suggestion:
| wait_for_sandbox_state "$secure_sandbox_id" "running" 30 | |
| if ! wait_for_sandbox_state "$secure_sandbox_id" "running" 30; then | |
| suite_summary "08_auth" || true | |
| exit 1 | |
| fi |
| suite_summary "08_auth" || true | ||
| exit 1 | ||
| fi | ||
| wait_for_sandbox_state "$other_secure_sandbox_id" "running" 30 |
There was a problem hiding this comment.
The second sandbox readiness result is also ignored. If this sandbox fails to become running, the token-scope request is made against an unready or absent envd and the test reports a misleading 401 rather than the provisioning failure. Check the return value before invoking proxy_envd_health.
Suggestion:
| wait_for_sandbox_state "$other_secure_sandbox_id" "running" 30 | |
| if ! wait_for_sandbox_state "$other_secure_sandbox_id" "running" 30; then | |
| suite_summary "08_auth" || true | |
| exit 1 | |
| fi |
| headers.remove(E2B_SANDBOX_ID_HEADER); | ||
| headers.remove(TARGET_PORT_HEADER); | ||
| headers.remove(E2B_TARGET_PORT_HEADER); | ||
| headers.remove(TRAFFIC_ACCESS_TOKEN_HEADER); |
There was a problem hiding this comment.
This removes the traffic token but still forwards Authorization and x-api-key to the sandbox application. In particular, require_auth only removes x-api-key when it is valid; a request authorized with a traffic token (or a public sandbox) can include an arbitrary API key and have it forwarded, while a bearer credential is always forwarded. These are AgentEnv control-plane/client credentials and become reusable secrets inside the sandbox. Remove control-plane authentication headers unconditionally before forwarding (unless there is an explicit, trusted application-level contract for them).
Suggestion:
| headers.remove(TRAFFIC_ACCESS_TOKEN_HEADER); | |
| headers.remove(TRAFFIC_ACCESS_TOKEN_HEADER); | |
| headers.remove(API_KEY_HEADER); | |
| headers.remove(header::AUTHORIZATION); |
| validate_directory_identity(parent).with_context(|| { | ||
| format!( | ||
| "validate managed secret directory ownership {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| set_permissions(parent, 0o700)?; | ||
| validate_directory(parent) | ||
| .with_context(|| format!("validate managed secret directory {}", parent.display()))?; | ||
|
|
||
| let mut temporary = tempfile::NamedTempFile::new_in(parent) |
There was a problem hiding this comment.
The directory checks are path-based and are separated from the subsequent filesystem operations. After validate_directory_identity/validate_directory, a same-UID process that can rename or replace parent can redirect set_permissions, NamedTempFile::new_in, persist_noclobber, or the directory sync to a different directory; the temporary secret could therefore be created under an unintended location. Anchor the operation to a directory file descriptor opened with no-follow semantics and use descriptor-relative operations (or otherwise hold/verify the directory identity across the whole create sequence).
| return Err(err).with_context(|| format!("stat {}", path.display())); | ||
| } | ||
| }; | ||
| let metadata = entry.metadata().await?; |
There was a problem hiding this comment.
A concurrent prune can delete this entry between next_entry and metadata, causing NotFound to abort the entire scan. Concurrent deletion is expected now that prune scans are no longer serialized. Handle ErrorKind::NotFound by continuing, while preserving context for other errors.
| match tokio::fs::remove_file(&path).await { | ||
| Ok(()) => total = total.saturating_sub(len), | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| ?err, | ||
| path = %entry.path.display(), | ||
| "remove premerged index artifact failed" | ||
| ) | ||
| tracing::warn!(?err, path = %path.display(), "remove premerged index artifact failed") | ||
| } | ||
| } |
There was a problem hiding this comment.
If another scan already removed path, NotFound means these recorded bytes are no longer present and must be subtracted from total. Leaving the stale length counted makes this scan continue deleting newer valid artifacts beyond the configured amount. Treat NotFound like successful removal; only warn for other errors.
Suggestion:
| match tokio::fs::remove_file(&path).await { | |
| Ok(()) => total = total.saturating_sub(len), | |
| Err(err) => { | |
| tracing::warn!( | |
| ?err, | |
| path = %entry.path.display(), | |
| "remove premerged index artifact failed" | |
| ) | |
| tracing::warn!(?err, path = %path.display(), "remove premerged index artifact failed") | |
| } | |
| } | |
| match tokio::fs::remove_file(&path).await { | |
| Ok(()) => total = total.saturating_sub(len), | |
| Err(err) if err.kind() == ErrorKind::NotFound => { | |
| total = total.saturating_sub(len); | |
| } | |
| Err(err) => { | |
| tracing::warn!(?err, path = %path.display(), "remove premerged index artifact failed") | |
| } | |
| } |
| if write_result.is_ok() { | ||
| let dir = cache_dir.join(PREMERGED_INDEX_DIR); | ||
| if let Err(err) = prune_premerged_index_dir(&dir, max_dir_bytes).await { |
There was a problem hiding this comment.
This now performs a full directory scan after every successful artifact write, and the per-directory scan gate was removed. Writers for different digests—and even a subsequent writer for the same digest after the lock is released—can therefore run overlapping scans and deletion passes. For a large cache or frequent merges, the per-entry async filesystem calls multiply metadata I/O and blocking-pool scheduling overhead. Retain per-directory serialization and an amortized growth threshold, or otherwise coalesce prune requests.
3 participants
Summary
AENV_API_KEY, then/run/secrets/api-key, then an atomically generated$AENV_HOME/secrets/api-key.envdAccessToken = hex(HMAC-SHA256(seed, sandboxID))trafficAccessToken = hex(HMAC-SHA256(seed, "sandbox-traffic-" + sandboxID))Credential boundaries
X-API-Key: deployment control-plane credential shared by the gateway and runtime nodes.e2b-traffic-access-token: sandbox-scoped application proxy credential.X-Access-Token: secure envd credential, accepted only for the matching sandbox envd port.The API key is independent from both sandbox credentials. Rotating an API key does not rotate sandbox tokens; rotating the runtime seed rotates both per-sandbox credentials.
Proxy routing strips AgentENV credentials before forwarding application requests and preserves application
Authorization.Deployment behavior
Secret/agentenv-auth. The existing optionalagentenv-runtime-secrets/sandbox-access-token-hash-seedcontract is unchanged, avoiding seed migration during upgrades.Out of scope