Skip to content

Commit 81d00f9

Browse files
committed
fix(server): keep exposed services off infrastructure errors
Exposed services do not authenticate callers, so they now reach an Error sandbox only when its main process failed. Exec, SSH, and forwarding keep the debugging path into an infrastructure Error. The CLI service forward stops again once its sandbox is stopping, stopped, or being deleted, and the TypeScript SDK forward applies the same check instead of requiring Ready. Exec refusals name what makes the sandbox usable again. supervisor_connected skips the owner index on a single replica, the unused relay error variant is gone, and relay opening outside sandbox_access is crate-private. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
1 parent 604025e commit 81d00f9

10 files changed

Lines changed: 202 additions & 56 deletions

File tree

‎crates/openshell-cli/src/run.rs‎

Lines changed: 59 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2260,10 +2260,12 @@ fn forward_requires_session_token(status: &Status) -> bool {
22602260
&& status.message().contains("authorization_token is required")
22612261
}
22622262

2263-
/// Confirm the sandbox still exists, returning the replica that owns it.
2263+
/// Confirm the sandbox can still serve the forward, returning the replica that
2264+
/// owns it.
22642265
///
22652266
/// The gateway decides whether each connection may reach the sandbox; this
2266-
/// only stops a forward whose sandbox is gone or going away.
2267+
/// only stops a forward whose sandbox is gone or is stopping, stopped, or
2268+
/// being deleted, since no connection can succeed until a new command acts.
22672269
async fn fetch_sandbox_for_forward(
22682270
client: &mut crate::tls::GrpcClient,
22692271
name: &str,
@@ -2293,15 +2295,25 @@ async fn fetch_sandbox_for_forward(
22932295
.sandbox
22942296
.ok_or_else(|| miette::miette!("sandbox '{name}' not found"))?;
22952297

2296-
if SandboxPhase::try_from(sandbox.phase()) == Ok(SandboxPhase::Deleting) {
2298+
if forward_ended_by_phase(sandbox.phase()) {
22972299
return Err(miette::miette!(
2298-
"sandbox '{name}' is being deleted; stopping service forward"
2300+
"sandbox '{name}' is no longer available (phase: {}); stopping service forward",
2301+
phase_name(sandbox.phase())
22992302
));
23002303
}
23012304

23022305
Ok(replica)
23032306
}
23042307

2308+
/// Phases in which no forwarded connection can succeed until another command
2309+
/// starts or recreates the sandbox.
2310+
fn forward_ended_by_phase(phase: i32) -> bool {
2311+
matches!(
2312+
SandboxPhase::try_from(phase),
2313+
Ok(SandboxPhase::Stopping | SandboxPhase::Stopped | SandboxPhase::Deleting)
2314+
)
2315+
}
2316+
23052317
#[derive(Debug)]
23062318
struct ForwardTcpConnectionError {
23072319
message: String,
@@ -2569,12 +2581,21 @@ impl Drop for TaskGuard {
25692581
}
25702582

25712583
/// The gateway decides whether a sandbox can be used. When it refuses, name the
2572-
/// phase this command saw so the user knows what to wait for.
2584+
/// phase this command saw and what would make the sandbox usable again.
25732585
fn exec_start_error(status: Status, sandbox: &Sandbox) -> miette::Report {
25742586
if status.code() == Code::FailedPrecondition && status.message() == "sandbox is not ready" {
2587+
let name = sandbox.object_name();
2588+
let hint = match SandboxPhase::try_from(sandbox.phase()) {
2589+
Ok(SandboxPhase::Stopping | SandboxPhase::Stopped) => {
2590+
format!("start it with `openshell sandbox start {name}`")
2591+
}
2592+
Ok(SandboxPhase::Completed | SandboxPhase::Error) => {
2593+
"its main process exited and its supervisor is no longer connected".to_string()
2594+
}
2595+
_ => "wait for it to reach Ready state".to_string(),
2596+
};
25752597
return miette::miette!(
2576-
"sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state",
2577-
sandbox.object_name(),
2598+
"sandbox '{name}' is not ready (phase: {}); {hint}",
25782599
phase_name(sandbox.phase())
25792600
);
25802601
}
@@ -7129,14 +7150,14 @@ fn format_endpoint(endpoint: &openshell_core::proto::NetworkEndpoint) -> String
71297150
mod tests {
71307151
use super::{
71317152
ForwardTcpConnectionError, PolicyGetView, ProvisioningStep, build_sandbox_resource_limits,
7132-
format_endpoint, format_log_line, forward_requires_session_token, git_sync_files,
7133-
has_main_process_result, parse_cli_setting_value, parse_credential_expiry_cli_value,
7134-
parse_driver_config_json, parse_env_from_pairs, parse_secret_material_env_pairs,
7135-
policy_revision_list_json, policy_revision_to_json, proto_execution_timeout,
7136-
provisioning_timeout_message, ready_false_condition_message, relay_local_socket,
7137-
resolve_from, rootfs_tar_sources_supported_for_gateway, sandbox_should_persist,
7138-
sandbox_upload_plan, service_endpoint_to_json, service_status_error,
7139-
service_url_for_gateway, workspace_member_to_json,
7153+
format_endpoint, format_log_line, forward_ended_by_phase, forward_requires_session_token,
7154+
git_sync_files, has_main_process_result, parse_cli_setting_value,
7155+
parse_credential_expiry_cli_value, parse_driver_config_json, parse_env_from_pairs,
7156+
parse_secret_material_env_pairs, policy_revision_list_json, policy_revision_to_json,
7157+
proto_execution_timeout, provisioning_timeout_message, ready_false_condition_message,
7158+
relay_local_socket, resolve_from, rootfs_tar_sources_supported_for_gateway,
7159+
sandbox_should_persist, sandbox_upload_plan, service_endpoint_to_json,
7160+
service_status_error, service_url_for_gateway, workspace_member_to_json,
71407161
};
71417162
use openshell_core::proto::TcpForwardFrame;
71427163

@@ -9503,4 +9524,27 @@ mod tests {
95039524
"authorization_token is required for ForwardTcp"
95049525
)));
95059526
}
9527+
9528+
#[test]
9529+
fn forward_leaves_reachability_after_exit_to_the_gateway() {
9530+
use openshell_core::proto::SandboxPhase;
9531+
9532+
for phase in [
9533+
SandboxPhase::Stopping,
9534+
SandboxPhase::Stopped,
9535+
SandboxPhase::Deleting,
9536+
] {
9537+
assert!(forward_ended_by_phase(phase as i32), "{phase:?}");
9538+
}
9539+
for phase in [
9540+
SandboxPhase::Provisioning,
9541+
SandboxPhase::Starting,
9542+
SandboxPhase::Ready,
9543+
SandboxPhase::Completed,
9544+
SandboxPhase::Error,
9545+
SandboxPhase::Suspended,
9546+
] {
9547+
assert!(!forward_ended_by_phase(phase as i32), "{phase:?}");
9548+
}
9549+
}
95069550
}

‎crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -528,8 +528,16 @@ async fn exec_names_the_phase_when_the_gateway_refuses() {
528528
drop(child.stdin.take());
529529
let output = finish(child).await;
530530
assert!(!output.status.success());
531-
let stderr = String::from_utf8_lossy(&output.stderr);
532-
assert!(stderr.contains("is not ready (phase: Stopped)"), "{stderr}");
531+
// The error renderer wraps long messages, so compare the words alone.
532+
let stderr = String::from_utf8_lossy(&output.stderr)
533+
.split_whitespace()
534+
.filter(|word| *word != "│")
535+
.collect::<Vec<_>>()
536+
.join(" ");
537+
assert!(
538+
stderr.contains("is not ready (phase: Stopped); start it with `openshell sandbox start"),
539+
"{stderr}"
540+
);
533541
assert!(gateway.calls.lock().unwrap().starts.is_empty());
534542
}
535543

‎crates/openshell-server/src/compute/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6879,7 +6879,7 @@ fn terminal_stop_exit_code(sandbox: &Sandbox) -> Option<i32> {
68796879
sandbox.status.as_ref()?.exit_code
68806880
}
68816881

6882-
fn is_failed_main_process_result(sandbox: &Sandbox) -> bool {
6882+
pub fn is_failed_main_process_result(sandbox: &Sandbox) -> bool {
68836883
sandbox.phase() == SandboxPhase::Error as i32
68846884
&& sandbox.status.as_ref().is_some_and(|status| {
68856885
status.exit_code.is_some()

‎crates/openshell-server/src/grpc/sandbox.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2853,8 +2853,9 @@ pub(super) async fn handle_forward_tcp(
28532853
.await?;
28542854

28552855
// The main process may finish between minting the SSH token and opening
2856-
// its transport. Keep the relay reachable until terminal delivery is
2857-
// finalized so fast commands can attach without a readiness race.
2856+
// its transport. The access rules keep a Completed or Error sandbox
2857+
// reachable while its supervisor is connected, so fast commands attach
2858+
// without a readiness race.
28582859
let access = crate::sandbox_access::acquire(state, &sandbox).await?;
28592860

28602861
let connection_guard =

‎crates/openshell-server/src/sandbox_access.rs‎

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
//! - A `Completed` or `Error` sandbox is usable while its supervisor is still
1818
//! connected to any replica. A retained sandbox keeps its access plane after
1919
//! its main process exits.
20+
//! - Exposed services, which do not authenticate callers, reach an `Error`
21+
//! sandbox only when its main process failed. An infrastructure failure
22+
//! stays reachable for debugging through the authenticated paths only.
2023
//! - Every other phase is refused.
2124
2225
use std::fmt;
@@ -42,16 +45,13 @@ pub enum AccessError {
4245
NotReachable,
4346
/// The gateway could not determine whether the sandbox is reachable.
4447
Lookup(Status),
45-
/// The supervisor did not open the relay.
46-
Relay(Status),
4748
}
4849

4950
impl fmt::Display for AccessError {
5051
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
5152
match self {
5253
Self::NotReachable => f.write_str("sandbox is not ready"),
5354
Self::Lookup(status) => write!(f, "sandbox access check failed: {status}"),
54-
Self::Relay(status) => write!(f, "supervisor relay failed: {status}"),
5555
}
5656
}
5757
}
@@ -61,7 +61,6 @@ impl From<AccessError> for Status {
6161
match err {
6262
AccessError::NotReachable => Self::failed_precondition(err.to_string()),
6363
AccessError::Lookup(status) => status,
64-
AccessError::Relay(_) => Self::unavailable(err.to_string()),
6564
}
6665
}
6766
}
@@ -95,14 +94,28 @@ pub async fn acquire(
9594
})
9695
}
9796

97+
/// Apply the access rules for an exposed service, which serves callers the
98+
/// gateway does not authenticate.
99+
pub async fn acquire_for_exposed_service(
100+
state: &Arc<ServerState>,
101+
sandbox: &Sandbox,
102+
) -> Result<SandboxAccess, AccessError> {
103+
if sandbox.phase() == SandboxPhase::Error as i32
104+
&& !crate::compute::is_failed_main_process_result(sandbox)
105+
{
106+
return Err(AccessError::NotReachable);
107+
}
108+
acquire(state, sandbox).await
109+
}
110+
98111
impl SandboxAccess {
99112
/// Ask the supervisor to open a relay to `target`, returning the channel
100113
/// id and a receiver that resolves once the supervisor claims it.
101114
pub async fn open_relay(
102115
&self,
103116
target: relay_open::Target,
104117
service_id: String,
105-
) -> Result<(String, oneshot::Receiver<Result<DuplexStream, Status>>), AccessError> {
118+
) -> Result<(String, oneshot::Receiver<Result<DuplexStream, Status>>), Status> {
106119
crate::supervisor_session::open_routed_relay_with_target(
107120
&self.state,
108121
&self.sandbox_id,
@@ -111,7 +124,7 @@ impl SandboxAccess {
111124
RELAY_SESSION_WAIT,
112125
)
113126
.await
114-
.map_err(AccessError::Relay)
127+
.map_err(|err| Status::unavailable(format!("supervisor relay failed: {err}")))
115128
}
116129
}
117130

@@ -187,6 +200,54 @@ mod tests {
187200
}
188201
}
189202

203+
fn error_sandbox(id: &str, reason: &str, exit_code: Option<i32>) -> Sandbox {
204+
let mut sandbox = sandbox_in(id, SandboxPhase::Error);
205+
sandbox.status = Some(openshell_core::proto::SandboxStatus {
206+
phase: SandboxPhase::Error.into(),
207+
exit_code,
208+
conditions: vec![openshell_core::proto::SandboxCondition {
209+
r#type: "Ready".into(),
210+
status: "False".into(),
211+
reason: reason.into(),
212+
..Default::default()
213+
}],
214+
..Default::default()
215+
});
216+
sandbox
217+
}
218+
219+
#[tokio::test]
220+
async fn exposed_services_skip_infrastructure_errors() {
221+
let state = crate::grpc::test_support::test_server_state().await;
222+
let (tx, _rx) = mpsc::channel(1);
223+
let (shutdown_tx, _shutdown_rx) = oneshot::channel();
224+
state.supervisor_sessions.register(
225+
"access".to_string(),
226+
"session".to_string(),
227+
tx,
228+
shutdown_tx,
229+
);
230+
let main_failed = error_sandbox("access", "MainProcessFailed", Some(3));
231+
let timed_out = error_sandbox("access", "ProvisioningTimedOut", None);
232+
233+
assert!(
234+
acquire_for_exposed_service(&state, &main_failed)
235+
.await
236+
.is_ok()
237+
);
238+
assert!(matches!(
239+
acquire_for_exposed_service(&state, &timed_out).await,
240+
Err(AccessError::NotReachable)
241+
));
242+
// Authenticated paths keep the debugging shell.
243+
assert!(acquire(&state, &timed_out).await.is_ok());
244+
assert!(
245+
acquire_for_exposed_service(&state, &sandbox_in("access", SandboxPhase::Completed))
246+
.await
247+
.is_ok()
248+
);
249+
}
250+
190251
#[test]
191252
fn not_reachable_keeps_the_existing_status() {
192253
let status = Status::from(AccessError::NotReachable);

‎crates/openshell-server/src/service_routing.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -730,12 +730,12 @@ async fn proxy_to_endpoint(
730730
};
731731
// Every request passes the access gate, including ones served from a
732732
// pooled upstream connection.
733-
let access = match sandbox_access::acquire(&state, &sandbox).await {
733+
let access = match sandbox_access::acquire_for_exposed_service(&state, &sandbox).await {
734734
Ok(access) => access,
735735
Err(err) => {
736736
let route_err = match err {
737737
AccessError::NotReachable => ServiceRouteError::sandbox_not_ready(),
738-
AccessError::Lookup(err) | AccessError::Relay(err) => {
738+
AccessError::Lookup(err) => {
739739
warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: failed to check sandbox access");
740740
ServiceRouteError::internal_error()
741741
}

0 commit comments

Comments
 (0)