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. |
1ab089a to
305e73f
Compare
305e73f to
92bcbad
Compare
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
| --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.
2 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