Skip to content

Commit 08a9d81

Browse files
committed
perf(server): drop per-connection session tokens for TCP forwards
`openshell forward service` minted an SSH session token before every forwarded TCP connection and revoked it afterwards: two store commits per connection. The token added nothing on that path. `ForwardTcp` already authenticates the caller and authorizes it against the sandbox's workspace on every stream before it looks at the token, the relay to the supervisor is opened with the sandbox id and target only, and the token is never forwarded, audited, or visible to the target service. The mechanism exists for `openshell sandbox ssh`, where the process that opens the stream is an ssh ProxyCommand holding nothing but the token. Reusing it per TCP connection put a store write on the connect path and serialized concurrent forwards on commit latency: #3494 measured the symptom, and #3543 made the commits cheaper, but each one still holds SQLite's writer lock for an fsync, so connection setup under a burst stayed linear in the number of concurrent connections. Let `target.tcp` streams omit `authorization_token`. The gateway admits them on the already-authorized principal, counts them against the same per-sandbox connection cap, and touches no store. `target.ssh` streams keep requiring the token. A token supplied with a TCP target is still validated and counted per token, so an older CLI against a new gateway is unchanged. The CLI stops minting and revoking a session per forwarded connection; against a gateway that predates this change it recognizes the `authorization_token is required` rejection once and falls back to per-connection tokens for the rest of the process. Tests cover token-less TCP admission and slot release, SSH targets still rejected without a token, a supplied token still validated, and the per-sandbox cap for token-less forwards; the CLI test pins the legacy detection predicate. Architecture and security docs describe which targets carry a token. Signed-off-by: Jason T. Greene <jason.greene@redhat.com>
1 parent a67567e commit 08a9d81

4 files changed

Lines changed: 315 additions & 66 deletions

File tree

‎architecture/gateway.md‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,10 +1132,15 @@ controls through an extended interface.
11321132

11331133
`ForwardTcp` is the client-facing byte stream for SSH and service forwarding.
11341134
The first frame is a `TcpForwardInit` that carries the workspace-scoped sandbox
1135-
name, an authorization token from `CreateSshSession`, and an explicit target:
1136-
`target.ssh` for the sandbox SSH socket or `target.tcp` for a loopback service
1137-
inside the sandbox. The gateway validates the token and sandbox readiness,
1138-
sends a targeted `RelayOpen` to the supervisor, then bridges
1135+
name and an explicit target: `target.ssh` for the sandbox SSH socket or
1136+
`target.tcp` for a loopback service inside the sandbox. The gateway authorizes
1137+
the caller's principal against the sandbox's workspace on every stream. SSH
1138+
targets additionally require the `authorization_token` issued by
1139+
`CreateSshSession`, because the process that opens them is an ssh
1140+
`ProxyCommand` that holds nothing else. TCP targets carry no token, so a
1141+
service forward costs no store access per connection and only the in-memory
1142+
per-sandbox connection cap applies. The gateway then checks sandbox readiness,
1143+
sends a targeted `RelayOpen` to the supervisor, and bridges
11391144
`TcpForwardFrame::Data` to `RelayFrame::Data` until either side closes.
11401145

11411146
Browser service URLs use the same supervisor relay path after host-based

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

Lines changed: 130 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -2024,20 +2024,6 @@ pub async fn service_forward_tcp(
20242024
let service_id = format!("service-forward:{name}:{target_host}:{target_port}");
20252025
let fatal_tx = fatal_tx.clone();
20262026
tokio::spawn(async move {
2027-
let token = match create_forward_session_token(
2028-
&mut client,
2029-
&sandbox_name,
2030-
&sandbox_workspace,
2031-
).await {
2032-
Ok(token) => token,
2033-
Err(err) => {
2034-
tracing::warn!(peer = %peer, error = %err, "service forward session creation failed");
2035-
if err.fatal {
2036-
let _ = fatal_tx.send(err.message).await;
2037-
}
2038-
return;
2039-
}
2040-
};
20412027
if let Err(err) = forward_one_tcp_connection(
20422028
&mut client,
20432029
socket,
@@ -2046,7 +2032,6 @@ pub async fn service_forward_tcp(
20462032
target_host,
20472033
target_port,
20482034
service_id,
2049-
token.clone(),
20502035
)
20512036
.await
20522037
{
@@ -2055,9 +2040,6 @@ pub async fn service_forward_tcp(
20552040
let _ = fatal_tx.send(err.message).await;
20562041
}
20572042
}
2058-
let _ = client
2059-
.revoke_ssh_session(RevokeSshSessionRequest { allow_missing: true, token })
2060-
.await;
20612043
});
20622044
}
20632045
}
@@ -2081,6 +2063,19 @@ async fn create_forward_session_token(
20812063
Ok(response.into_inner().token)
20822064
}
20832065

2066+
/// Set once this process learns that the gateway predates principal-authorized
2067+
/// TCP forwards and still requires a `CreateSshSession` token per connection.
2068+
static LEGACY_FORWARD_SESSION_TOKENS: std::sync::atomic::AtomicBool =
2069+
std::sync::atomic::AtomicBool::new(false);
2070+
2071+
/// Older gateways reject a token-less `ForwardTcp` init with this
2072+
/// `Unauthenticated` status; newer ones authorize TCP targets on the caller's
2073+
/// principal and only demand a token for SSH targets.
2074+
fn forward_requires_session_token(status: &Status) -> bool {
2075+
status.code() == Code::Unauthenticated
2076+
&& status.message().contains("authorization_token is required")
2077+
}
2078+
20842079
async fn fetch_ready_sandbox_for_forward(
20852080
client: &mut crate::tls::GrpcClient,
20862081
name: &str,
@@ -2182,37 +2177,118 @@ async fn forward_one_tcp_connection(
21822177
target_host: String,
21832178
target_port: u16,
21842179
service_id: String,
2185-
authorization_token: String,
21862180
) -> std::result::Result<(), ForwardTcpConnectionError> {
2187-
use tokio::io::{AsyncReadExt, AsyncWriteExt};
2181+
let mut init = TcpForwardInit {
2182+
sandbox: sandbox_name.clone(),
2183+
workspace: workspace.clone(),
2184+
service_id,
2185+
target: Some(tcp_forward_init::Target::Tcp(TcpRelayTarget {
2186+
host: target_host,
2187+
port: u32::from(target_port),
2188+
})),
2189+
// The gateway authorizes TCP forwards on this client's credentials for
2190+
// every stream, so no per-connection session token is minted unless the
2191+
// gateway turns out to predate that (`forward_requires_session_token`).
2192+
authorization_token: String::new(),
2193+
};
2194+
2195+
let mut session_token = None;
2196+
if LEGACY_FORWARD_SESSION_TOKENS.load(std::sync::atomic::Ordering::Relaxed) {
2197+
match create_forward_session_token(client, &sandbox_name, &workspace).await {
2198+
Ok(token) => {
2199+
init.authorization_token.clone_from(&token);
2200+
session_token = Some(token);
2201+
}
2202+
Err(err) => {
2203+
drain_and_shutdown_local_socket(socket).await;
2204+
return Err(err);
2205+
}
2206+
}
2207+
}
2208+
2209+
let opened = match open_forward_tcp_stream(client, init.clone()).await {
2210+
Ok(opened) => opened,
2211+
Err(status) if session_token.is_none() && forward_requires_session_token(&status) => {
2212+
tracing::info!(
2213+
"gateway requires an SSH session token per forwarded connection; \
2214+
minting one per connection for the rest of this forward"
2215+
);
2216+
LEGACY_FORWARD_SESSION_TOKENS.store(true, std::sync::atomic::Ordering::Relaxed);
2217+
let token = match create_forward_session_token(client, &sandbox_name, &workspace).await
2218+
{
2219+
Ok(token) => token,
2220+
Err(err) => {
2221+
drain_and_shutdown_local_socket(socket).await;
2222+
return Err(err);
2223+
}
2224+
};
2225+
init.authorization_token.clone_from(&token);
2226+
session_token = Some(token);
2227+
match open_forward_tcp_stream(client, init).await {
2228+
Ok(opened) => opened,
2229+
Err(status) => {
2230+
drain_and_shutdown_local_socket(socket).await;
2231+
revoke_forward_session_token(client, session_token).await;
2232+
return Err(ForwardTcpConnectionError::from_status(status));
2233+
}
2234+
}
2235+
}
2236+
Err(status) => {
2237+
drain_and_shutdown_local_socket(socket).await;
2238+
revoke_forward_session_token(client, session_token).await;
2239+
return Err(ForwardTcpConnectionError::from_status(status));
2240+
}
2241+
};
2242+
2243+
let result = bridge_local_socket_to_forward_stream(socket, opened).await;
2244+
revoke_forward_session_token(client, session_token).await;
2245+
result
2246+
}
2247+
2248+
/// An open `ForwardTcp` stream: the sender for local-to-gateway frames and the
2249+
/// gateway-to-local response stream.
2250+
type OpenForwardTcpStream = (
2251+
tokio::sync::mpsc::Sender<TcpForwardFrame>,
2252+
tonic::Streaming<TcpForwardFrame>,
2253+
);
2254+
2255+
async fn open_forward_tcp_stream(
2256+
client: &mut crate::tls::GrpcClient,
2257+
init: TcpForwardInit,
2258+
) -> std::result::Result<OpenForwardTcpStream, Status> {
21882259
use tokio_stream::wrappers::ReceiverStream;
21892260

21902261
let (tx, rx) = tokio::sync::mpsc::channel::<TcpForwardFrame>(16);
21912262
tx.send(TcpForwardFrame {
21922263
payload: Some(openshell_core::proto::tcp_forward_frame::Payload::Init(
2193-
TcpForwardInit {
2194-
sandbox: sandbox_name,
2195-
workspace: workspace.clone(),
2196-
service_id,
2197-
target: Some(tcp_forward_init::Target::Tcp(TcpRelayTarget {
2198-
host: target_host,
2199-
port: u32::from(target_port),
2200-
})),
2201-
authorization_token,
2202-
},
2264+
init,
22032265
)),
22042266
})
22052267
.await
2206-
.map_err(|_| ForwardTcpConnectionError::transient("failed to initialize forward stream"))?;
2268+
.map_err(|_| Status::internal("failed to initialize forward stream"))?;
2269+
let response = client
2270+
.forward_tcp(ReceiverStream::new(rx))
2271+
.await?
2272+
.into_inner();
2273+
Ok((tx, response))
2274+
}
22072275

2208-
let mut response = match client.forward_tcp(ReceiverStream::new(rx)).await {
2209-
Ok(response) => response.into_inner(),
2210-
Err(status) => {
2211-
let err = ForwardTcpConnectionError::from_status(status);
2212-
drain_and_shutdown_local_socket(socket).await;
2213-
return Err(err);
2214-
}
2215-
};
2276+
async fn revoke_forward_session_token(client: &mut crate::tls::GrpcClient, token: Option<String>) {
2277+
if let Some(token) = token {
2278+
let _ = client
2279+
.revoke_ssh_session(RevokeSshSessionRequest {
2280+
allow_missing: true,
2281+
token,
2282+
})
2283+
.await;
2284+
}
2285+
}
2286+
2287+
async fn bridge_local_socket_to_forward_stream(
2288+
socket: tokio::net::TcpStream,
2289+
(tx, mut response): OpenForwardTcpStream,
2290+
) -> std::result::Result<(), ForwardTcpConnectionError> {
2291+
use tokio::io::{AsyncReadExt, AsyncWriteExt};
22162292

22172293
let (mut local_read, mut local_write) = socket.into_split();
22182294

@@ -6402,8 +6478,8 @@ fn format_endpoint(endpoint: &openshell_core::proto::NetworkEndpoint) -> String
64026478
mod tests {
64036479
use super::{
64046480
PolicyGetView, ProvisioningStep, build_sandbox_resource_limits, format_endpoint,
6405-
format_log_line, git_sync_files, has_main_process_result, parse_cli_setting_value,
6406-
parse_credential_expiry_cli_value, parse_driver_config_json,
6481+
format_log_line, forward_requires_session_token, git_sync_files, has_main_process_result,
6482+
parse_cli_setting_value, parse_credential_expiry_cli_value, parse_driver_config_json,
64076483
parse_secret_material_env_pairs, policy_revision_list_json, policy_revision_to_json,
64086484
proto_execution_timeout, provisioning_timeout_message, ready_false_condition_message,
64096485
resolve_from, rootfs_tar_sources_supported_for_gateway, sandbox_should_persist,
@@ -8052,4 +8128,17 @@ mod tests {
80528128
let log = log_line("OCSF", "ocsf", message, "sandbox", &[]);
80538129
assert!(format_log_line(&log).ends_with(message));
80548130
}
8131+
8132+
#[test]
8133+
fn forward_requires_session_token_matches_only_the_legacy_gateway_error() {
8134+
assert!(forward_requires_session_token(&Status::unauthenticated(
8135+
"authorization_token is required for ForwardTcp"
8136+
)));
8137+
assert!(!forward_requires_session_token(&Status::unauthenticated(
8138+
"SSH session token not found"
8139+
)));
8140+
assert!(!forward_requires_session_token(&Status::permission_denied(
8141+
"authorization_token is required for ForwardTcp"
8142+
)));
8143+
}
80558144
}

0 commit comments

Comments
 (0)