feat(ssh): real SSH access via guest sessiond and runtime stream API#965
feat(ssh): real SSH access via guest sessiond and runtime stream API#965nieyy wants to merge 1 commit into
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an end-to-end SSH session platform: a shared session-frame protocol, guest session daemon, runtime and SDK APIs, Runner upgrade bridge, public SSH gateway, API validation fields, binary packaging, and protocol and integration tests. ChangesSSH session platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/box/services/box.service.ts (1)
1545-1578: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPossible crash on stale
SshAccess.boxrelation; runner lookup is dead code.The final fallback (Line 1577) accesses
sshAccess.box.idunconditionally, but the precedingif (sshAccess.box && sshAccess.box.runnerId)guard impliessshAccess.boxcan be falsy. If the box row has been hard-deleted (e.g.cleanupDestroyedBoxes) while a longer-livedSshAccessrow still points at it, this throws instead of returning{valid: false}. Also, the innerrunnerlookup and itsif (runner)branch return an identical payload to the fallback line, so it accomplishes nothing but obscures the missing null-check.🛡️ Proposed fix
- // Get runner information if box exists - if (sshAccess.box && sshAccess.box.runnerId) { - const runner = await this.runnerService.findOne(sshAccess.box.runnerId) - - if (runner) { - return { - valid: true, - boxId: sshAccess.box.id, - unixUser: SSH_ACCESS_UNIX_USER, - tokenId: sshAccess.id, - } - } - } - - return { valid: true, boxId: sshAccess.box.id, unixUser: SSH_ACCESS_UNIX_USER, tokenId: sshAccess.id } + if (!sshAccess.box) { + return { valid: false, boxId: null } + } + + return { + valid: true, + boxId: sshAccess.box.id, + unixUser: SSH_ACCESS_UNIX_USER, + tokenId: sshAccess.id, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/box/services/box.service.ts` around lines 1545 - 1578, Handle a missing box relation in validateSshAccess by returning { valid: false, boxId: null } when sshAccess.box is absent. Remove the redundant runnerService.findOne lookup and its identical success branch; after confirming the box exists, return the valid payload using sshAccess.box.id.
🧹 Nitpick comments (4)
src/ssh-gateway/src/runner.rs (1)
57-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRedact the bearer token in
RunnerClient'sDebugimpl.The derived
Debugprints the fullbearerstring (including the service token) if the struct is ever logged. While no current log site triggers this, the derived impl makes accidental leakage trivial.Gatewayalready usesfinish_non_exhaustive()for the same reason.🔒️ Proposed custom Debug impl
-#[derive(Debug, Clone)] +#[derive(Clone)] pub(crate) struct RunnerClient { bearer: String, timeout: Duration, } +impl std::fmt::Debug for RunnerClient { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RunnerClient") + .field("bearer", &"[redacted]") + .field("timeout", &self.timeout) + .finish() + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ssh-gateway/src/runner.rs` around lines 57 - 61, Replace the derived Debug implementation on RunnerClient with a custom fmt::Debug implementation that omits or redacts the bearer token while retaining safe fields such as timeout; use finish_non_exhaustive() or an equivalent redacted representation, matching Gateway’s approach.src/ssh-gateway/src/server.rs (1)
169-181: 🩺 Stability & Availability | 🔵 TrivialConsider bounding concurrent connections in the accept loop.
runspawns a tokio task per accepted connection with no concurrency cap. Under a connection flood, this leads to unbounded task growth and potential resource exhaustion. ASemaphoreor connection counter would cap concurrent sessions. Alternatively, rely on upstream load-balancer/firewall connection limits if the gateway is never directly exposed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ssh-gateway/src/server.rs` around lines 169 - 181, Add a concurrency limit to Server::run around each accepted connection, using a Tokio Semaphore or equivalent counter initialized to the configured maximum; acquire a permit before spawning serve_stream, move the permit into the task so it is released when the session ends, and define the behavior for permit acquisition or exhaustion without allowing unbounded task creation.src/boxlite/src/disk/ext4.rs (1)
448-470: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPotential debugfs stdin/stderr pipe deadlock on large command batches or many failures.
stdin.write_all()blocks until the whole command script is delivered before any stderr is read viawait_with_output().normalize_inodes_with_debugfscan generate a command batch far exceeding typical pipe buffer sizes for real container images (many thousands of files), and ifdebugfsneeds to write enough error output to fill its stderr pipe while still consuming stdin, both processes can stall indefinitely — defeating the "fail loudly, never hang" intent of this fix.♻️ Proposed fix: write stdin from a separate thread so stderr can be drained concurrently
- if let Some(mut stdin) = child.stdin.take() { - stdin.write_all(commands.as_bytes()).map_err(|e| { - BoxliteError::Storage(format!("Failed to write to debugfs stdin: {}", e)) - })?; - } - - let output = child - .wait_with_output() - .map_err(|e| BoxliteError::Storage(format!("Failed to wait for debugfs: {}", e)))?; + let mut stdin = child.stdin.take(); + let commands_owned = commands.to_string(); + let writer = std::thread::spawn(move || -> std::io::Result<()> { + if let Some(mut stdin) = stdin.take() { + stdin.write_all(commands_owned.as_bytes())?; + } + Ok(()) + }); + + let output = child + .wait_with_output() + .map_err(|e| BoxliteError::Storage(format!("Failed to wait for debugfs: {}", e)))?; + writer + .join() + .map_err(|_| BoxliteError::Storage("debugfs stdin writer thread panicked".into()))? + .map_err(|e| BoxliteError::Storage(format!("Failed to write to debugfs stdin: {}", e)))?;Since this depends on OS pipe-buffer semantics and
debugfs's actual stdin/stderr interleaving behavior, worth confirming against the target platform's default pipe size and typical image file counts before prioritizing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/disk/ext4.rs` around lines 448 - 470, The run_debugfs_commands function can deadlock by writing the entire command batch before draining debugfs stderr. Move stdin.write_all into a separate writer thread (closing stdin afterward), then concurrently wait_with_output to drain stderr; join the writer and propagate any write or wait errors through the existing BoxliteError::Storage handling.src/sessiond/src/main.rs (1)
87-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd backoff on persistent accept errors to prevent busy-waiting.
If
listener.accept()consistently returnsErr(e.g., vsock device removed), the loop spins without yielding effectively, logging warnings and consuming CPU. A small sleep on error mitigates this.♻️ Proposed fix
Err(e) => { warn!(error = %e, "vsock accept failed"); + tokio::time::sleep(Duration::from_millis(100)).await; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sessiond/src/main.rs` around lines 87 - 102, Add a short asynchronous delay in the Err branch of the listener.accept() loop, after logging the warning, so persistent accept failures do not cause busy-waiting. Update the loop surrounding listener.accept() while preserving the existing successful connection handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/clean.sh`:
- Around line 120-123: Quote the $GUEST_TARGET portion of every path in the rm
-rf command, including the pre-existing argument, so each target remains a
single literal path and avoids word splitting or glob expansion.
In `@sdks/c/src/session.rs`:
- Around line 194-208: In the session readiness handling, guard
CBoxSessionError::from_session_error(&reason) with an out_reason null check
before constructing it, and apply the same pattern in the corresponding
out_session_err path around lines 266-270. Only call write_session_error with a
constructed error when the caller provided a non-NULL output pointer.
In `@src/guest/src/service/server.rs`:
- Around line 93-100: Stop the newly created Sessiond when Sessiond::start()
cannot be stored because the OnceLock is already initialized. Capture the
returned Sessiond in the set failure path and call its stop() method before
discarding it, while preserving the existing warning and successful
initialization behavior.
In `@src/sessiond/src/server.rs`:
- Around line 318-319: Remove raw command content from the debug log in the exec
handler. Update the `debug!` call near the `exec requested` log to record only
the command length, preserving diagnostic value without exposing secrets and
matching the `env_request` handler’s redaction behavior.
In `@src/ssh-gateway/src/frames.rs`:
- Around line 189-203: Update register_channel to acquire the routes mutex
before checking self.closed, then return MuxClosed while holding that lock when
shutdown has begun; only create and insert the channel after the guarded check
so registration cannot occur after read_loop clears the routes.
---
Outside diff comments:
In `@apps/api/src/box/services/box.service.ts`:
- Around line 1545-1578: Handle a missing box relation in validateSshAccess by
returning { valid: false, boxId: null } when sshAccess.box is absent. Remove the
redundant runnerService.findOne lookup and its identical success branch; after
confirming the box exists, return the valid payload using sshAccess.box.id.
---
Nitpick comments:
In `@src/boxlite/src/disk/ext4.rs`:
- Around line 448-470: The run_debugfs_commands function can deadlock by writing
the entire command batch before draining debugfs stderr. Move stdin.write_all
into a separate writer thread (closing stdin afterward), then concurrently
wait_with_output to drain stderr; join the writer and propagate any write or
wait errors through the existing BoxliteError::Storage handling.
In `@src/sessiond/src/main.rs`:
- Around line 87-102: Add a short asynchronous delay in the Err branch of the
listener.accept() loop, after logging the warning, so persistent accept failures
do not cause busy-waiting. Update the loop surrounding listener.accept() while
preserving the existing successful connection handling.
In `@src/ssh-gateway/src/runner.rs`:
- Around line 57-61: Replace the derived Debug implementation on RunnerClient
with a custom fmt::Debug implementation that omits or redacts the bearer token
while retaining safe fields such as timeout; use finish_non_exhaustive() or an
equivalent redacted representation, matching Gateway’s approach.
In `@src/ssh-gateway/src/server.rs`:
- Around line 169-181: Add a concurrency limit to Server::run around each
accepted connection, using a Tokio Semaphore or equivalent counter initialized
to the configured maximum; acquire a permit before spawning serve_stream, move
the permit into the task so it is released when the session ends, and define the
behavior for permit acquisition or exhaustion without allowing unbounded task
creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62bde688-b9c9-4f39-be4d-2ffccf601941
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (92)
.github/workflows/build-c.yml.github/workflows/build-node.yml.github/workflows/build-runtime.yml.github/workflows/build-wheels.yml.github/workflows/warm-caches.ymlCargo.tomlapps/api-client-go/api/openapi.yamlapps/api-client-go/model_ssh_access_validation_dto.goapps/api/src/box/controllers/box.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/services/box.service.ssh-access.spec.tsapps/api/src/box/services/box.service.tsapps/libs/api-client/src/docs/SshAccessValidationDto.mdapps/libs/api-client/src/models/ssh-access-validation-dto.tsapps/runner/pkg/api/controllers/boxlite_ssh.goapps/runner/pkg/api/controllers/boxlite_ssh_test.goapps/runner/pkg/api/server.goapps/runner/pkg/sessionbridge/bridge.goapps/runner/pkg/sessionbridge/bridge_test.goapps/runner/pkg/sessionbridge/channel.goapps/runner/pkg/sessionbridge/guest_test.goapps/runner/pkg/sessionframe/codec.goapps/runner/pkg/sessionframe/codec_test.goapps/runner/pkg/sessionframe/frame.goapps/runner/pkg/sessionframe/payload.goapps/runner/pkg/sessionframe/payload_test.godocs/architecture/ssh-session-frame-protocol.mdscripts/build/build-guest.shscripts/build/build-runtime.shscripts/clean.shsdks/c/include/boxlite.hsdks/c/src/lib.rssdks/c/src/session.rssdks/go/session.gosdks/go/session_integration_test.gosdks/go/session_test.gosrc/boxlite/build.rssrc/boxlite/src/disk/ext4.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/tasks/guest_rootfs.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/session.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/shim.rssrc/boxlite/src/vmm/krun/engine.rssrc/boxlite/src/vmm/mod.rssrc/boxlite/tests/session.rssrc/deps/e2fsprogs-sys/build.rssrc/guest/Cargo.tomlsrc/guest/src/main.rssrc/guest/src/service/delegation.rssrc/guest/src/service/guest.rssrc/guest/src/service/mod.rssrc/guest/src/service/server.rssrc/guest/src/sessiond.rssrc/session-frame/Cargo.tomlsrc/session-frame/src/frame.rssrc/session-frame/src/lib.rssrc/session-frame/src/payload.rssrc/session-frame/src/stream.rssrc/session-frame/tests/golden_vectors.rssrc/sessiond/Cargo.tomlsrc/sessiond/src/bridge.rssrc/sessiond/src/exec.rssrc/sessiond/src/lib.rssrc/sessiond/src/listen.rssrc/sessiond/src/main.rssrc/sessiond/src/server.rssrc/sessiond/tests/ssh_session.rssrc/shared/src/constants.rssrc/ssh-gateway/Cargo.tomlsrc/ssh-gateway/README.mdsrc/ssh-gateway/src/config.rssrc/ssh-gateway/src/frames.rssrc/ssh-gateway/src/http.rssrc/ssh-gateway/src/lib.rssrc/ssh-gateway/src/main.rssrc/ssh-gateway/src/metrics.rssrc/ssh-gateway/src/redact.rssrc/ssh-gateway/src/runner.rssrc/ssh-gateway/src/server.rssrc/ssh-gateway/src/token.rssrc/ssh-gateway/tests/common/mod.rssrc/ssh-gateway/tests/gateway_e2e.rssrc/ssh-gateway/tests/token_validator.rs
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
| pub async fn run(&self, listener: TcpListener) -> std::io::Result<()> { | ||
| loop { | ||
| let (stream, peer) = listener.accept().await?; | ||
| let _ = stream.set_nodelay(true); | ||
| debug!(peer = %peer, "TCP connection accepted"); | ||
| let gateway = self.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(e) = gateway.serve_stream(stream).await { | ||
| debug!(error = %e, "connection task ended with error"); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Gateway::run spawns an unauthenticated tokio task per accepted TCP connection with no semaphore/rate-limit/per-IP cap, so a remote attacker can exhaust fds/memory before any auth check runs.
There was a problem hiding this comment.
Fixed — the accept loop now gates each connection behind a tokio::sync::Semaphore sized by the new GatewayConfig::max_connections (default 4096, env BOXLITE_SSH_MAX_CONNECTIONS); over-cap connections are dropped immediately after accept(), before any auth work, and the rejection is counted in route_failures{connection_limit}. Added run_drops_connections_beyond_the_configured_cap (real TcpListener) as a regression test.
| #[derive(Debug, Clone, Parser)] | ||
| #[command(name = "boxlite-ssh-gateway", about = "BoxLite public SSH gateway")] | ||
| pub struct GatewayConfig { | ||
| /// TCP address the public SSH listener binds. | ||
| #[arg(long, env = "BOXLITE_SSH_LISTEN_ADDR", default_value = "0.0.0.0:2222")] | ||
| pub listen_addr: SocketAddr, | ||
|
|
||
| /// Path of the persistent ed25519 host key. Generated once if absent and | ||
| /// then reused forever: public clients pin this key, so it must never be | ||
| /// ephemeral. | ||
| #[arg(long, env = "BOXLITE_SSH_HOST_KEY_PATH")] | ||
| pub host_key_path: PathBuf, | ||
|
|
||
| /// Base URL of the Hosted API (token validation + runner resolution). | ||
| #[arg(long, env = "BOXLITE_HOSTED_API_URL")] | ||
| pub hosted_api_url: String, | ||
|
|
||
| /// Service credential sent as `Authorization: Bearer` to the Hosted API. | ||
| #[arg(long, env = "BOXLITE_HOSTED_API_TOKEN", hide_env_values = true)] | ||
| pub hosted_api_token: String, | ||
|
|
||
| /// Internal service token sent as `Authorization: Bearer` to Runners. | ||
| #[arg(long, env = "BOXLITE_RUNNER_SERVICE_TOKEN", hide_env_values = true)] | ||
| pub runner_service_token: String, |
There was a problem hiding this comment.
🧹 Secret fields under derive(Debug)
GatewayConfig, HttpHostedApi and RunnerClient derive Debug over plain-String hosted_api_token/runner_service_token/bearer fields; no current call site logs {:?} on them, but any future debug!(?config) would leak full credentials, unlike TokenValidator/Gateway which have manual Debug impls.
There was a problem hiding this comment.
Fixed — GatewayConfig, RunnerClient, and HttpHostedApi no longer derive Debug; each now has a manual impl fmt::Debug that redacts the token/bearer fields to <redacted>, so a future debug!(?config) can't leak them. Added a test on each asserting the raw secret never appears in format!("{:?}", ...).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ssh-gateway-russh/src/frames.rs (1)
189-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
register_channelcan still race with shutdown — same issue as previously flagged.The
closedcheck (line 193) is outside therouteslock, so a channel can be inserted afterread_loophas already cleared the map (line 321). The returned receiver will hang until theFrameMuxitself is dropped. This is the same race condition flagged in the prior review ofsrc/ssh-gateway/src/frames.rs:189-203(now renamed tosrc/ssh-gateway-russh/src/frames.rs).Move the
closedcheck inside therouteslock so registration is atomic with the shutdown clear:🔒 Proposed fix
pub(crate) fn register_channel( &self, channel_id: u32, ) -> Result<mpsc::Receiver<Frame>, MuxClosed> { - if self.closed.load(Ordering::Relaxed) { - return Err(MuxClosed); - } let (tx, rx) = mpsc::channel(CHANNEL_QUEUE_DEPTH); - self.routes - .lock() - .expect("routes mutex poisoned") - .channels - .insert(channel_id, tx); + let mut routes = self.routes.lock().expect("routes mutex poisoned"); + if self.closed.load(Ordering::Relaxed) { + return Err(MuxClosed); + } + routes.channels.insert(channel_id, tx); Ok(rx) }This ensures
register_channeleither seesclosed = true(returnsErr) or inserts the channel beforeread_loopclears routes (the channel is then properly closed when routes are cleared, andrx.recv()returnsNonepromptly). Theread_loopordering (closed.store(true)before acquiring the lock) is unchanged, sosendstill seesclosed = truebeforeShutdownis enqueued.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ssh-gateway-russh/src/frames.rs` around lines 189 - 203, register_channel checks shutdown state outside the routes lock, allowing registration to race with read_loop cleanup. Acquire the routes mutex before checking closed, return MuxClosed while holding that lock when closed, and insert the sender only when open so registration is atomic with shutdown clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/ssh-gateway-russh/src/frames.rs`:
- Around line 189-203: register_channel checks shutdown state outside the routes
lock, allowing registration to race with read_loop cleanup. Acquire the routes
mutex before checking closed, return MuxClosed while holding that lock when
closed, and insert the sender only when open so registration is atomic with
shutdown clearing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 343389dc-68e0-418e-bb9b-e6dffbf20e91
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (93)
.github/workflows/build-c.yml.github/workflows/build-node.yml.github/workflows/build-runtime.yml.github/workflows/build-wheels.yml.github/workflows/warm-caches.yml.gitignoreCargo.tomlapps/api-client-go/api/openapi.yamlapps/api-client-go/model_ssh_access_validation_dto.goapps/api/src/box/controllers/box.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/services/box.service.ssh-access.spec.tsapps/api/src/box/services/box.service.tsapps/libs/api-client/src/docs/SshAccessValidationDto.mdapps/libs/api-client/src/models/ssh-access-validation-dto.tsapps/runner/pkg/api/controllers/boxlite_ssh.goapps/runner/pkg/api/controllers/boxlite_ssh_test.goapps/runner/pkg/api/server.goapps/runner/pkg/sessionbridge/bridge.goapps/runner/pkg/sessionbridge/bridge_test.goapps/runner/pkg/sessionbridge/channel.goapps/runner/pkg/sessionbridge/guest_test.goapps/runner/pkg/sessionframe/codec.goapps/runner/pkg/sessionframe/codec_test.goapps/runner/pkg/sessionframe/frame.goapps/runner/pkg/sessionframe/payload.goapps/runner/pkg/sessionframe/payload_test.godocs/architecture/ssh-session-frame-protocol.mdscripts/build/build-guest.shscripts/build/build-runtime.shscripts/clean.shsdks/c/include/boxlite.hsdks/c/src/lib.rssdks/c/src/session.rssdks/go/session.gosdks/go/session_integration_test.gosdks/go/session_test.gosrc/boxlite/build.rssrc/boxlite/src/disk/ext4.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/tasks/guest_rootfs.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/session.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/shim.rssrc/boxlite/src/vmm/krun/engine.rssrc/boxlite/src/vmm/mod.rssrc/boxlite/tests/session.rssrc/deps/e2fsprogs-sys/build.rssrc/guest/Cargo.tomlsrc/guest/src/main.rssrc/guest/src/service/delegation.rssrc/guest/src/service/guest.rssrc/guest/src/service/mod.rssrc/guest/src/service/server.rssrc/guest/src/sessiond.rssrc/session-frame/Cargo.tomlsrc/session-frame/src/frame.rssrc/session-frame/src/lib.rssrc/session-frame/src/payload.rssrc/session-frame/src/stream.rssrc/session-frame/tests/golden_vectors.rssrc/sessiond/Cargo.tomlsrc/sessiond/src/bridge.rssrc/sessiond/src/exec.rssrc/sessiond/src/lib.rssrc/sessiond/src/listen.rssrc/sessiond/src/main.rssrc/sessiond/src/server.rssrc/sessiond/tests/ssh_session.rssrc/shared/src/constants.rssrc/ssh-gateway-russh/Cargo.tomlsrc/ssh-gateway-russh/README.mdsrc/ssh-gateway-russh/src/config.rssrc/ssh-gateway-russh/src/frames.rssrc/ssh-gateway-russh/src/http.rssrc/ssh-gateway-russh/src/lib.rssrc/ssh-gateway-russh/src/main.rssrc/ssh-gateway-russh/src/metrics.rssrc/ssh-gateway-russh/src/redact.rssrc/ssh-gateway-russh/src/runner.rssrc/ssh-gateway-russh/src/server.rssrc/ssh-gateway-russh/src/token.rssrc/ssh-gateway-russh/tests/common/mod.rssrc/ssh-gateway-russh/tests/gateway_e2e.rssrc/ssh-gateway-russh/tests/token_validator.rs
✅ Files skipped from review due to trivial changes (5)
- .github/workflows/build-wheels.yml
- .gitignore
- apps/libs/api-client/src/docs/SshAccessValidationDto.md
- src/boxlite/src/litebox/init/tasks/guest_rootfs.rs
- apps/libs/api-client/src/models/ssh-access-validation-dto.ts
🚧 Files skipped from review as they are similar to previous changes (64)
- apps/api/src/box/dto/ssh-access.dto.ts
- src/boxlite/src/runtime/layout.rs
- src/session-frame/Cargo.toml
- Cargo.toml
- src/boxlite/src/disk/mod.rs
- src/guest/src/service/guest.rs
- .github/workflows/build-c.yml
- src/sessiond/src/listen.rs
- apps/api-client-go/model_ssh_access_validation_dto.go
- sdks/c/src/lib.rs
- .github/workflows/build-runtime.yml
- apps/runner/pkg/api/server.go
- src/sessiond/src/lib.rs
- docs/architecture/ssh-session-frame-protocol.md
- src/boxlite/src/vmm/krun/engine.rs
- src/boxlite/src/lib.rs
- .github/workflows/build-node.yml
- src/session-frame/tests/golden_vectors.rs
- apps/runner/pkg/sessionframe/payload.go
- src/boxlite/src/vmm/mod.rs
- .github/workflows/warm-caches.yml
- src/sessiond/Cargo.toml
- apps/runner/pkg/sessionframe/payload_test.go
- src/session-frame/src/payload.rs
- src/deps/e2fsprogs-sys/build.rs
- src/boxlite/src/runtime/backend.rs
- src/guest/src/service/mod.rs
- src/guest/src/service/server.rs
- src/session-frame/src/lib.rs
- apps/api/src/box/services/box.service.ts
- src/guest/src/service/delegation.rs
- src/sessiond/src/main.rs
- src/guest/Cargo.toml
- src/boxlite/src/litebox/mod.rs
- apps/api/src/box/controllers/box.controller.ts
- sdks/go/session_integration_test.go
- src/boxlite/tests/session.rs
- src/session-frame/src/stream.rs
- src/shared/src/constants.rs
- src/guest/src/sessiond.rs
- sdks/go/session.go
- apps/api/src/box/services/box.service.ssh-access.spec.ts
- src/session-frame/src/frame.rs
- src/sessiond/src/exec.rs
- src/boxlite/src/net/socket_path.rs
- src/boxlite/src/util/mod.rs
- src/boxlite/src/vmm/controller/shim.rs
- src/sessiond/src/bridge.rs
- sdks/go/session_test.go
- src/boxlite/src/litebox/init/tasks/vmm_spawn.rs
- apps/runner/pkg/api/controllers/boxlite_ssh.go
- src/boxlite/src/disk/ext4.rs
- scripts/build/build-guest.sh
- src/sessiond/src/server.rs
- sdks/c/include/boxlite.h
- src/boxlite/src/litebox/box_impl.rs
- src/boxlite/build.rs
- src/guest/src/main.rs
- sdks/c/src/session.rs
- src/sessiond/tests/ssh_session.rs
- src/boxlite/src/rootfs/guest.rs
- src/boxlite/src/litebox/session.rs
- apps/runner/pkg/sessionbridge/bridge.go
- apps/runner/pkg/sessionbridge/bridge_test.go
| session::ensure_supported_service(service).map_err(|e| { | ||
| BoxSessionError::new( | ||
| BoxSessionErrorCode::Internal, | ||
| BoxSessionPhase::SessionOpen, | ||
| self.id().as_str(), | ||
| e.to_string(), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
session_ready (line 348) surfaces InvalidArgument for a bad service string while open_session_stream (366-372) remaps the identical error to Internal, so the C/Go SDK sees different error codes for the same bad input depending on entry point; no test covers this.
There was a problem hiding this comment.
Fixed — this was a real inconsistency, thanks for catching it. open_session_stream no longer folds an unsupported service into BoxSessionError{code: Internal}; both entry points now share the same error class for the same bad input via a new OpenSessionError { Argument(BoxliteError), Session(BoxSessionError) } split — Argument (e.g. an unsupported service) reports InvalidArgument through out_error only, exactly like session_ready already did; Session keeps the existing typed session-failure taxonomy. Threaded through the trait, both backends, the public re-exports, and the C FFI match arms (no ABI change — verified the regenerated header diff is doc-comment only). Added a real-VM regression test plus two unit tests, and two-side verified: reverting the fix makes the new tests fail to compile with 'cannot find OpenSessionError'.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sessiond/tests/ssh_session.rs`:
- Around line 78-96: Fix the notification race between wait_for and mutate by
replacing changed.notify_waiters() with changed.notify_one() in mutate,
preserving the existing state mutation and return behavior so notifications are
retained when no waiter is registered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d4f1cc3-bcc6-4fcd-ba09-db81b9e75702
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (93)
.github/workflows/build-c.yml.github/workflows/build-node.yml.github/workflows/build-runtime.yml.github/workflows/build-wheels.yml.github/workflows/warm-caches.yml.gitignoreCargo.tomlapps/api-client-go/api/openapi.yamlapps/api-client-go/model_ssh_access_validation_dto.goapps/api/src/box/controllers/box.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/services/box.service.ssh-access.spec.tsapps/api/src/box/services/box.service.tsapps/libs/api-client/src/docs/SshAccessValidationDto.mdapps/libs/api-client/src/models/ssh-access-validation-dto.tsapps/runner/pkg/api/controllers/boxlite_ssh.goapps/runner/pkg/api/controllers/boxlite_ssh_test.goapps/runner/pkg/api/server.goapps/runner/pkg/sessionbridge/bridge.goapps/runner/pkg/sessionbridge/bridge_test.goapps/runner/pkg/sessionbridge/channel.goapps/runner/pkg/sessionbridge/guest_test.goapps/runner/pkg/sessionframe/codec.goapps/runner/pkg/sessionframe/codec_test.goapps/runner/pkg/sessionframe/frame.goapps/runner/pkg/sessionframe/payload.goapps/runner/pkg/sessionframe/payload_test.godocs/architecture/ssh-session-frame-protocol.mdscripts/build/build-guest.shscripts/build/build-runtime.shscripts/clean.shsdks/c/include/boxlite.hsdks/c/src/lib.rssdks/c/src/session.rssdks/go/session.gosdks/go/session_integration_test.gosdks/go/session_test.gosrc/boxlite/build.rssrc/boxlite/src/disk/ext4.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/tasks/guest_rootfs.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/session.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/shim.rssrc/boxlite/src/vmm/krun/engine.rssrc/boxlite/src/vmm/mod.rssrc/boxlite/tests/session.rssrc/deps/e2fsprogs-sys/build.rssrc/guest/Cargo.tomlsrc/guest/src/main.rssrc/guest/src/service/delegation.rssrc/guest/src/service/guest.rssrc/guest/src/service/mod.rssrc/guest/src/service/server.rssrc/guest/src/sessiond.rssrc/session-frame/Cargo.tomlsrc/session-frame/src/frame.rssrc/session-frame/src/lib.rssrc/session-frame/src/payload.rssrc/session-frame/src/stream.rssrc/session-frame/tests/golden_vectors.rssrc/sessiond/Cargo.tomlsrc/sessiond/src/bridge.rssrc/sessiond/src/exec.rssrc/sessiond/src/lib.rssrc/sessiond/src/listen.rssrc/sessiond/src/main.rssrc/sessiond/src/server.rssrc/sessiond/tests/ssh_session.rssrc/shared/src/constants.rssrc/ssh-gateway-russh/Cargo.tomlsrc/ssh-gateway-russh/README.mdsrc/ssh-gateway-russh/src/config.rssrc/ssh-gateway-russh/src/frames.rssrc/ssh-gateway-russh/src/http.rssrc/ssh-gateway-russh/src/lib.rssrc/ssh-gateway-russh/src/main.rssrc/ssh-gateway-russh/src/metrics.rssrc/ssh-gateway-russh/src/redact.rssrc/ssh-gateway-russh/src/runner.rssrc/ssh-gateway-russh/src/server.rssrc/ssh-gateway-russh/src/token.rssrc/ssh-gateway-russh/tests/common/mod.rssrc/ssh-gateway-russh/tests/gateway_e2e.rssrc/ssh-gateway-russh/tests/token_validator.rs
✅ Files skipped from review due to trivial changes (7)
- src/guest/src/main.rs
- src/boxlite/src/disk/mod.rs
- src/boxlite/src/litebox/init/tasks/guest_rootfs.rs
- .gitignore
- .github/workflows/warm-caches.yml
- .github/workflows/build-c.yml
- apps/libs/api-client/src/models/ssh-access-validation-dto.ts
🚧 Files skipped from review as they are similar to previous changes (74)
- .github/workflows/build-wheels.yml
- src/guest/src/service/mod.rs
- src/session-frame/Cargo.toml
- scripts/clean.sh
- src/ssh-gateway-russh/Cargo.toml
- apps/runner/pkg/sessionframe/payload.go
- sdks/c/src/lib.rs
- .github/workflows/build-runtime.yml
- Cargo.toml
- src/sessiond/src/lib.rs
- apps/runner/pkg/api/server.go
- apps/api/src/box/controllers/box.controller.ts
- apps/libs/api-client/src/docs/SshAccessValidationDto.md
- src/deps/e2fsprogs-sys/build.rs
- src/sessiond/Cargo.toml
- src/session-frame/src/payload.rs
- src/sessiond/src/exec.rs
- src/session-frame/tests/golden_vectors.rs
- src/boxlite/src/lib.rs
- src/ssh-gateway-russh/src/redact.rs
- docs/architecture/ssh-session-frame-protocol.md
- apps/api/src/box/services/box.service.ts
- apps/api/src/box/dto/ssh-access.dto.ts
- src/session-frame/src/lib.rs
- src/guest/src/service/delegation.rs
- apps/api/src/box/services/box.service.ssh-access.spec.ts
- apps/api-client-go/model_ssh_access_validation_dto.go
- sdks/c/include/boxlite.h
- apps/runner/pkg/sessionframe/payload_test.go
- src/guest/src/service/guest.rs
- src/shared/src/constants.rs
- src/boxlite/src/util/mod.rs
- src/boxlite/src/vmm/controller/shim.rs
- src/guest/Cargo.toml
- sdks/go/session_integration_test.go
- src/boxlite/src/litebox/mod.rs
- src/sessiond/src/bridge.rs
- .github/workflows/build-node.yml
- src/ssh-gateway-russh/src/main.rs
- src/boxlite/src/runtime/backend.rs
- src/guest/src/sessiond.rs
- src/boxlite/src/litebox/init/tasks/vmm_spawn.rs
- src/boxlite/src/vmm/krun/engine.rs
- src/ssh-gateway-russh/tests/token_validator.rs
- src/boxlite/src/litebox/box_impl.rs
- src/session-frame/src/stream.rs
- src/boxlite/src/vmm/mod.rs
- scripts/build/build-guest.sh
- src/session-frame/src/frame.rs
- src/boxlite/src/net/socket_path.rs
- src/boxlite/src/runtime/layout.rs
- src/guest/src/service/server.rs
- src/ssh-gateway-russh/src/metrics.rs
- sdks/c/src/session.rs
- src/ssh-gateway-russh/src/token.rs
- sdks/go/session_test.go
- src/sessiond/src/main.rs
- sdks/go/session.go
- src/ssh-gateway-russh/src/http.rs
- src/ssh-gateway-russh/src/runner.rs
- apps/runner/pkg/api/controllers/boxlite_ssh.go
- src/ssh-gateway-russh/src/config.rs
- apps/runner/pkg/sessionbridge/bridge.go
- src/ssh-gateway-russh/src/lib.rs
- src/ssh-gateway-russh/src/frames.rs
- src/sessiond/src/listen.rs
- src/boxlite/build.rs
- src/ssh-gateway-russh/src/server.rs
- apps/runner/pkg/sessionbridge/bridge_test.go
- src/sessiond/src/server.rs
- src/ssh-gateway-russh/tests/gateway_e2e.rs
- src/boxlite/src/disk/ext4.rs
- src/ssh-gateway-russh/tests/common/mod.rs
- src/boxlite/src/rootfs/guest.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api-client-go/api/openapi.yaml`:
- Around line 6819-6827: Update the SSH access-token validation response schema
around the existing valid/unixUser/tokenId fields to use separate oneOf
variants: require valid: true together with boxId, unixUser, and tokenId in the
success shape, while keeping the invalid variant minimal with valid: false.
Ensure generated clients derive these fields as required only for successful
responses.
In `@src/ssh-gateway-russh/README.md`:
- Around line 21-25: Update the public-leg validation flow described in the
README and its implementation to stop sending the opaque SSH access token as the
`token` URL query parameter; transmit it through a dedicated redacted header or
request body instead, while keeping the gateway service credential in its
separate authentication mechanism. Update the Hosted API contract and any
related client/server handlers to read the token from that new location,
preserving validation and replay protections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7af7a707-c3cc-421b-a8a8-ecefb1407a2e
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (93)
.github/workflows/build-c.yml.github/workflows/build-node.yml.github/workflows/build-runtime.yml.github/workflows/build-wheels.yml.github/workflows/warm-caches.yml.gitignoreCargo.tomlapps/api-client-go/api/openapi.yamlapps/api-client-go/model_ssh_access_validation_dto.goapps/api/src/box/controllers/box.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/services/box.service.ssh-access.spec.tsapps/api/src/box/services/box.service.tsapps/libs/api-client/src/docs/SshAccessValidationDto.mdapps/libs/api-client/src/models/ssh-access-validation-dto.tsapps/runner/pkg/api/controllers/boxlite_ssh.goapps/runner/pkg/api/controllers/boxlite_ssh_test.goapps/runner/pkg/api/server.goapps/runner/pkg/sessionbridge/bridge.goapps/runner/pkg/sessionbridge/bridge_test.goapps/runner/pkg/sessionbridge/channel.goapps/runner/pkg/sessionbridge/guest_test.goapps/runner/pkg/sessionframe/codec.goapps/runner/pkg/sessionframe/codec_test.goapps/runner/pkg/sessionframe/frame.goapps/runner/pkg/sessionframe/payload.goapps/runner/pkg/sessionframe/payload_test.godocs/architecture/ssh-session-frame-protocol.mdscripts/build/build-guest.shscripts/build/build-runtime.shscripts/clean.shsdks/c/include/boxlite.hsdks/c/src/lib.rssdks/c/src/session.rssdks/go/session.gosdks/go/session_integration_test.gosdks/go/session_test.gosrc/boxlite/build.rssrc/boxlite/src/disk/ext4.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/tasks/guest_rootfs.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/session.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/shim.rssrc/boxlite/src/vmm/krun/engine.rssrc/boxlite/src/vmm/mod.rssrc/boxlite/tests/session.rssrc/deps/e2fsprogs-sys/build.rssrc/guest/Cargo.tomlsrc/guest/src/main.rssrc/guest/src/service/delegation.rssrc/guest/src/service/guest.rssrc/guest/src/service/mod.rssrc/guest/src/service/server.rssrc/guest/src/sessiond.rssrc/session-frame/Cargo.tomlsrc/session-frame/src/frame.rssrc/session-frame/src/lib.rssrc/session-frame/src/payload.rssrc/session-frame/src/stream.rssrc/session-frame/tests/golden_vectors.rssrc/sessiond/Cargo.tomlsrc/sessiond/src/bridge.rssrc/sessiond/src/exec.rssrc/sessiond/src/lib.rssrc/sessiond/src/listen.rssrc/sessiond/src/main.rssrc/sessiond/src/server.rssrc/sessiond/tests/ssh_session.rssrc/shared/src/constants.rssrc/ssh-gateway-russh/Cargo.tomlsrc/ssh-gateway-russh/README.mdsrc/ssh-gateway-russh/src/config.rssrc/ssh-gateway-russh/src/frames.rssrc/ssh-gateway-russh/src/http.rssrc/ssh-gateway-russh/src/lib.rssrc/ssh-gateway-russh/src/main.rssrc/ssh-gateway-russh/src/metrics.rssrc/ssh-gateway-russh/src/redact.rssrc/ssh-gateway-russh/src/runner.rssrc/ssh-gateway-russh/src/server.rssrc/ssh-gateway-russh/src/token.rssrc/ssh-gateway-russh/tests/common/mod.rssrc/ssh-gateway-russh/tests/gateway_e2e.rssrc/ssh-gateway-russh/tests/token_validator.rs
✅ Files skipped from review due to trivial changes (8)
- .github/workflows/build-wheels.yml
- src/session-frame/Cargo.toml
- .github/workflows/build-c.yml
- apps/libs/api-client/src/docs/SshAccessValidationDto.md
- src/boxlite/src/disk/mod.rs
- .github/workflows/build-runtime.yml
- src/boxlite/src/litebox/init/tasks/guest_rootfs.rs
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (75)
- .github/workflows/warm-caches.yml
- apps/libs/api-client/src/models/ssh-access-validation-dto.ts
- apps/api/src/box/controllers/box.controller.ts
- src/ssh-gateway-russh/src/lib.rs
- src/deps/e2fsprogs-sys/build.rs
- src/guest/src/service/guest.rs
- src/boxlite/src/litebox/init/tasks/vmm_spawn.rs
- Cargo.toml
- src/sessiond/src/lib.rs
- src/boxlite/src/lib.rs
- src/sessiond/Cargo.toml
- sdks/c/src/lib.rs
- src/session-frame/src/payload.rs
- docs/architecture/ssh-session-frame-protocol.md
- src/ssh-gateway-russh/Cargo.toml
- src/guest/Cargo.toml
- apps/runner/pkg/sessionframe/payload.go
- apps/runner/pkg/sessionframe/payload_test.go
- src/guest/src/service/mod.rs
- .github/workflows/build-node.yml
- src/boxlite/src/vmm/controller/shim.rs
- apps/api/src/box/services/box.service.ssh-access.spec.ts
- src/boxlite/tests/session.rs
- scripts/clean.sh
- apps/api/src/box/dto/ssh-access.dto.ts
- src/boxlite/src/vmm/krun/engine.rs
- sdks/go/session_test.go
- src/ssh-gateway-russh/src/redact.rs
- src/ssh-gateway-russh/src/main.rs
- sdks/go/session.go
- src/boxlite/src/runtime/layout.rs
- src/session-frame/tests/golden_vectors.rs
- apps/runner/pkg/api/server.go
- src/sessiond/src/main.rs
- src/session-frame/src/lib.rs
- src/ssh-gateway-russh/src/metrics.rs
- src/sessiond/src/listen.rs
- src/guest/src/main.rs
- src/boxlite/src/vmm/mod.rs
- scripts/build/build-guest.sh
- src/shared/src/constants.rs
- src/session-frame/src/stream.rs
- src/guest/src/service/delegation.rs
- sdks/go/session_integration_test.go
- src/guest/src/sessiond.rs
- src/ssh-gateway-russh/tests/token_validator.rs
- apps/api-client-go/model_ssh_access_validation_dto.go
- sdks/c/include/boxlite.h
- apps/runner/pkg/api/controllers/boxlite_ssh.go
- src/ssh-gateway-russh/src/config.rs
- src/sessiond/src/exec.rs
- src/boxlite/src/net/socket_path.rs
- src/boxlite/src/util/mod.rs
- src/guest/src/service/server.rs
- src/boxlite/build.rs
- src/sessiond/src/server.rs
- src/ssh-gateway-russh/src/token.rs
- apps/api/src/box/services/box.service.ts
- src/sessiond/src/bridge.rs
- src/sessiond/tests/ssh_session.rs
- src/boxlite/src/runtime/backend.rs
- src/ssh-gateway-russh/src/frames.rs
- src/session-frame/src/frame.rs
- src/boxlite/src/rootfs/guest.rs
- src/ssh-gateway-russh/src/http.rs
- src/boxlite/src/litebox/mod.rs
- src/boxlite/src/litebox/box_impl.rs
- src/ssh-gateway-russh/src/runner.rs
- apps/runner/pkg/sessionbridge/bridge.go
- src/boxlite/src/litebox/session.rs
- src/ssh-gateway-russh/src/server.rs
- apps/runner/pkg/sessionbridge/bridge_test.go
- sdks/c/src/session.rs
- src/boxlite/src/disk/ext4.rs
- src/ssh-gateway-russh/tests/gateway_e2e.rs
| unixUser: | ||
| description: Guest unix user the SSH session runs as | ||
| example: root | ||
| type: string | ||
| tokenId: | ||
| description: "Non-sensitive identifier of the SSH access token, for audit\ | ||
| \ correlation" | ||
| example: 123e4567-e89b-12d3-a456-426614174000 | ||
| type: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'SshAccessValidationDto|unixUser|tokenId' \
apps/api/src/box \
apps/api-client-go \
apps/libs/api-clientRepository: boxlite-ai/boxlite
Length of output: 43896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '6804,6845p' apps/api-client-go/api/openapi.yaml
printf '\n--- DTO ---\n'
sed -n '92,131p' apps/api/src/box/dto/ssh-access.dto.ts
printf '\n--- generator model required fields ---\n'
sed -n '190,238p' apps/api-client-go/model_ssh_access_validation_dto.goRepository: boxlite-ai/boxlite
Length of output: 3734
Model the success response as a separate shape. valid: true still allows unixUser and tokenId to be omitted, and the generated clients treat them as optional. Split this into oneOf variants so the valid branch requires boxId, unixUser, and tokenId, while the invalid branch stays minimal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api-client-go/api/openapi.yaml` around lines 6819 - 6827, Update the SSH
access-token validation response schema around the existing
valid/unixUser/tokenId fields to use separate oneOf variants: require valid:
true together with boxId, unixUser, and tokenId in the success shape, while
keeping the invalid variant minimal with valid: false. Ensure generated clients
derive these fields as required only for successful responses.
There was a problem hiding this comment.
Verified the invariant holds: every valid: true code path in BoxService.validateSshAccess (box.service.ts) sets unixUser/tokenId unconditionally — there's no valid: true branch that omits them. So the oneOf split would accurately document real behavior, not just tighten types speculatively.
Not doing it in this PR though: it needs a SshAccessValidationDto split into two @ApiExtraModels-refed classes plus a full regen of the generated clients (apps/api-client-go, apps/libs/api-client, apps/libs/analytics-api-client via the yarn nx run *:generate:api-client pipeline, which boots the NestJS app against Redis and needs Java 21 for openapi-generator-cli) — the api-client-drift.yml check fails on any source/generated mismatch, so a partial edit here would just break that CI job. No Java toolchain in this environment to regenerate and verify locally. Filing as a follow-up rather than landing a half-applied change.
There was a problem hiding this comment.
@nieyy Thanks for verifying the invariant and for the detailed explanation of the regeneration constraints — that makes sense, and deferring this to a follow-up rather than landing a partial edit is the right call given the api-client-drift.yml gate.
Would you like me to open a follow-up issue to track the SshAccessValidationDto oneOf split (splitting into success/failure variants via @ApiExtraModels) plus the corresponding client regeneration across apps/api-client-go, apps/libs/api-client, and apps/libs/analytics-api-client? Just let me know and I can file it with this context.
| 1. **Public leg** — real SSH. The client authenticates with the `none` method; | ||
| the username (the token) is validated via | ||
| `GET /box/ssh-access/validate?token=…` on the Hosted API, then the Runner | ||
| is resolved via `GET /runners/by-box/{boxId}` (the runner's `domain` field, | ||
| mirroring the legacy Go gateway). Password/publickey attempts are rejected |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not send the SSH credential in the URL query.
token=… can be captured by Hosted API, proxy, and tracing access logs; anyone with that value can replay it until expiry. Move it to a dedicated redacted header or request body, while preserving the gateway service credential separately.
Based on the PR objective that opaque access tokens grant end-to-end SSH access.
🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: Ensure spelling is correct
Context: ...roring the legacy Go gateway). Password/publickey attempts are rejected and the client...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ssh-gateway-russh/README.md` around lines 21 - 25, Update the public-leg
validation flow described in the README and its implementation to stop sending
the opaque SSH access token as the `token` URL query parameter; transmit it
through a dedicated redacted header or request body instead, while keeping the
gateway service credential in its separate authentication mechanism. Update the
Hosted API contract and any related client/server handlers to read the token
from that new location, preserving validation and replay protections.
There was a problem hiding this comment.
Confirmed accurate, but this mirrors a pre-existing contract, not something new here. GET /box/ssh-access/validate?token=… is the same endpoint shape the legacy Go gateway already uses in production (apps/ssh-gateway/README.md:108, @Query('token') in box.controller.ts predates this PR) — this gateway deliberately matches it (see the "mirrors the legacy Go gateway" note in token.rs's module doc).
The call itself is server-to-server (gateway → Hosted API, both sides authenticated with the SSH_GATEWAY_API_KEY service bearer, never touching the public internet), so the practical exposure is Hosted API/proxy/tracing access logs rather than a public-facing leak — real risk, but the same risk the existing Go gateway already carries today.
Fixing it means changing the shared Hosted API contract (adding a header-based alternative, or moving off query params) in a way that's coordinated across both gateways, since the endpoint is already live behind the legacy gateway. That's a larger, cross-cutting change beyond "add the new Rust gateway alongside the existing one" — tracking it as a follow-up rather than expanding this PR's scope.
There was a problem hiding this comment.
@nieyy, that's a fair call — since the query-param token contract predates this PR and is shared with the legacy Go gateway, fixing it here would mean changing a live, cross-cutting Hosted API contract rather than something scoped to this new gateway. Tracking it as a follow-up makes sense to avoid scope creep.
Would you like me to open a follow-up GitHub issue to track moving the SSH access token off the query string (e.g., to a header) across both the legacy Go gateway and this new Rust gateway? I can link this PR/comment and summarize the required coordinated changes if so.
nieyy
force-pushed
the
nieyy/russh
branch
2 times, most recently
from
8a61bed to
0b52d85
Compare
| func (b *Bridge) handleStdin(f *sessionframe.Frame) { | ||
| ch := b.lookupChannel(f.ChannelID) | ||
| if ch == nil { | ||
| return | ||
| } | ||
| ch.mu.Lock() | ||
| stdin := ch.stdin | ||
| closed := ch.stdinClosed | ||
| ch.mu.Unlock() | ||
| if stdin == nil || closed { | ||
| return | ||
| } | ||
| if _, err := stdin.Write(f.Payload); err != nil { | ||
| b.logger.Debug("stdin write failed", "channel", f.ChannelID, "error", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
handleStdin writes synchronously to ssh.Session stdin inside the single readLoop goroutine (bridge.go:196-211); if one channel's guest process stops draining stdin, the blocking Write freezes dispatch for every other concurrently open channel on that connection, confirmed multi-channel by bridge_test.go:280-306.
There was a problem hiding this comment.
Fixed — handleStdin no longer writes to the guest's stdin pipe directly from dispatch/readLoop. Each bridgeChannel now has a bounded stdinCh (depth 32) drained by a dedicated stdinWriter goroutine; handleStdin does a non-blocking select/default send and drops on overflow (logged) instead of blocking the shared dispatch path. EOF is routed through the same channel (stdinEOF, closed not sent-to) so stdinWriter drains any already-queued bytes before closing the pipe — avoids a write/close race I hit on the first pass (caught by -race + TestBridgeStdinAndHalfClose failing with "hello " instead of "hello guest").
Added TestBridgeStalledChannelDoesNotBlockSiblingChannel: floods one channel's stdin past the inner SSH session's default 2 MiB window (guest never reads), then asserts a sibling channel still opens and completes within its normal timeout. Verified against a revert of the fix — the test genuinely times out (i/o timeout after 10s) on the old synchronous-write code, and passes in ~10ms with the fix. Full sessionbridge suite (14 tests) passes under -race, repeated 3x.
| async fn data( | ||
| &mut self, | ||
| channel: ChannelId, | ||
| data: &[u8], | ||
| _session: &mut Session, | ||
| ) -> Result<(), Self::Error> { | ||
| if let Some(bridge) = self.channels.get_mut(&channel) { | ||
| bridge.stdin(data).await; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
data handler awaits a bounded mpsc send (bridge.rs:220-240, depth 32) per channel from one sequential per-connection dispatcher; a stalled stdin consumer on one channel can back-pressure the queue and stall other channels on the same SSH connection, not independently confirmed against russh's scheduler.
There was a problem hiding this comment.
Fixed — same class of fix as the Go-side finding. ChannelBridge::stdin() (bridge.rs) now uses stdin_tx.try_send(frame) instead of .send(frame).await, so a full queue drops the frame (logged) instead of blocking the shared per-connection dispatch loop (russh::server::Handler methods are called sequentially for one connection, confirmed in russh-0.62.2/src/server/encrypted.rs: handler.data(...).await sits inline in the same match arm every other channel's frames go through). stdin_eof() still must never be silently lost (the guest would hang waiting for more input), so it's spawned as its own task doing the blocking send — tokio::sync::mpsc guarantees that either resolves once the queue drains or fails immediately if the consumer is already gone, so it can't leak.
I did try to add a "one stalled channel doesn't block its sibling" integration regression test here too, matching the Go one, but backed it out: under a single-threaded test runtime it reliably reproduced the pre-fix hang, but under a multi-threaded runtime the same test passed even with the fix reverted back to blocking .send().await — the test was actually being confounded by shared-gRPC-connection buffering and Tokio scheduler timing (all RPCs for a boxlite-guest connection share one tonic::transport::Channel), not reliably exercising the one line I changed. Rather than land a flaky/misleading test, I'm relying on: the fix being a direct, verifiable non-blocking API guarantee (try_send never awaits — that's tokio::sync::mpsc's documented contract, not something that needs empirical proof), plus the full existing sessiond suite (14 tests, stdin_is_forwarded_then_closed_on_eof and concurrent_channels_get_distinct_executions_and_routed_output included) passing unchanged, plus cargo fmt/clippy -D warnings clean.
| session, stdin, err := ch.openSession(command, pty) | ||
| if err != nil { | ||
| b.removeChannel(f.ChannelID) | ||
| reply(err, codeOpenFailed) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Failed channel open does not burn channel id
When openSession fails, removeChannel runs but b.used[id] is never set, letting a later OPEN_* frame reuse the same channel id, contradicting the stated per-connection-uniqueness invariant on b.used.
There was a problem hiding this comment.
Fixed — removeChannel now unconditionally sets b.used[id] = true alongside the delete(b.channels, id), so every removal path (failed open, peer close, normal exit) permanently burns the id, not just the successful-open path. Added TestBridgeChannelReuseRejectedAfterFailedOpen, which opens a channel with an unregistered guest command (so openSession fails), then confirms a reopen of the same id is still rejected with CHANNEL_REUSED. Verified against a revert: without the fix the test fails with {"ok":true} (reuse silently accepted); with it, all 13 sessionbridge tests pass.
| impl HostedApi for HttpHostedApi { | ||
| fn validate_ssh_access<'a>(&'a self, token: &'a str) -> ApiFuture<'a, SshAccessValidation> { | ||
| Box::pin(async move { | ||
| let path = format!("/box/ssh-access/validate?token={}", percent_encode(token)); |
There was a problem hiding this comment.
Token (sole SSH auth credential, valid up to configured expiry) is placed in the GET query string to the Hosted API, risking exposure via app/proxy/WAF access logs during its validity window; mirrors legacy Go gateway's existing contract but the new client reproduces rather than fixes the pattern.
There was a problem hiding this comment.
Same finding as an earlier CodeRabbit comment on this PR (README.md, resolved thread) — see my reply there: #965 (comment)
Short version: confirmed accurate, but this is the legacy Go gateway's pre-existing contract (apps/ssh-gateway, @Query('token') in box.controller.ts predates this PR), not something new here — the new gateway deliberately mirrors it. Fixing it means a coordinated change to the shared Hosted API contract across both gateways, which is out of scope for "add the new Rust gateway alongside the existing one." Tracking as a follow-up.
| tokio::spawn(async move { | ||
| if let Err(e) = server.serve_stream(stream).await { | ||
| warn!(error = %e, "SSH connection ended with error"); | ||
| } |
There was a problem hiding this comment.
🧹 vsock accept loop lacks error backoff
serve_vsock has no backoff on repeated accept() errors (e.g. FD exhaustion), could busy-loop briefly under a persistent kernel error.
There was a problem hiding this comment.
Fixed — serve_vsock now backs off on repeated accept() errors instead of busy-looping: starts at 10ms, doubles up to a 1s cap, resets to 10ms on the next successful accept (same idiom as Go's net/http.Server.Serve). Verified via cargo check/clippy -D warnings on aarch64-unknown-linux-musl (this module is #[cfg(target_os = "linux")]-gated, so it can't be built/tested on the dev host directly) — both clean.
There was a problem hiding this comment.
Follow-up: my first fix was only compile/lint-verified, not behavior-tested — extracted the backoff math (next_accept_error_backoff, ACCEPT_ERROR_BACKOFF_MIN/MAX) into plain functions/consts outside the #[cfg(target_os = "linux")] module (gated #[cfg(any(test, target_os = "linux"))] so they don't trip dead-code lints on a non-Linux, non-test build) and added 3 unit tests: doubling per step, capping at 1s over 64 iterations (no overflow), and MIN < MAX. All pass natively on the dev host now (cargo test -p boxlite-sessiond, no cross-compilation needed for the tests themselves). serve_vsock itself still just calls these. Re-verified cargo check/clippy -D warnings --all-targets clean on both the native target and aarch64-unknown-linux-musl, and cargo fmt --check clean.
| async fn pump_output( | ||
| mut client: ExecutionClient<GrpcChannel>, | ||
| execution_id: String, | ||
| write_half: ChannelWriteHalf<Msg>, | ||
| finished: Arc<AtomicBool>, | ||
| ) { | ||
| let attach_request = AttachRequest { | ||
| execution_id: execution_id.clone(), | ||
| }; | ||
| match client.attach(attach_request).await { | ||
| Ok(response) => { | ||
| let mut output = response.into_inner(); | ||
| loop { | ||
| match output.message().await { | ||
| Ok(Some(chunk)) => { | ||
| let write_result = match chunk.event { | ||
| Some(exec_output::Event::Stdout(stdout)) => { | ||
| write_half.data_bytes(stdout.data).await | ||
| } | ||
| Some(exec_output::Event::Stderr(stderr)) => { | ||
| write_half | ||
| .extended_data_bytes(SSH_EXTENDED_DATA_STDERR, stderr.data) | ||
| .await | ||
| } | ||
| None => Ok(()), | ||
| }; | ||
| if write_result.is_err() { | ||
| debug!(execution_id = %execution_id, "SSH channel gone; stopping output pump"); | ||
| break; | ||
| } | ||
| } | ||
| Ok(None) => break, | ||
| Err(e) => { | ||
| warn!(execution_id = %execution_id, error = %e, "Attach stream failed"); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| warn!(execution_id = %execution_id, error = %e, "Attach RPC failed"); | ||
| } | ||
| } | ||
|
|
||
| let wait_request = WaitRequest { | ||
| execution_id: execution_id.clone(), | ||
| }; | ||
| let exit_status = match client.wait(wait_request).await { | ||
| Ok(response) => { | ||
| let wait = response.into_inner(); | ||
| if wait.signal != 0 { | ||
| SIGNAL_EXIT_BASE + wait.signal as u32 | ||
| } else { | ||
| wait.exit_code as u32 | ||
| } | ||
| } | ||
| Err(e) => { | ||
| warn!(execution_id = %execution_id, error = %e, "Wait RPC failed; reporting indeterminate exit"); | ||
| INDETERMINATE_EXIT_STATUS | ||
| } | ||
| }; | ||
| finished.store(true, Ordering::SeqCst); | ||
| info!(execution_id = %execution_id, exit_status, "delegated execution finished"); | ||
|
|
||
| let _ = write_half.exit_status(exit_status).await; | ||
| let _ = write_half.eof().await; | ||
| let _ = write_half.close().await; | ||
| } |
There was a problem hiding this comment.
Spawned at bridge.rs:202 with no timeout/cancellation token; if the SSH channel closes, kill_running()'s Kill RPC times out (10s, warn-only, no retry, bridge.rs:351-353), and the delegated guest process is wedged and produces no more output, output.message().await at line 379 blocks indefinitely, leaking the Attach gRPC stream and ChannelWriteHalf per stuck session.
There was a problem hiding this comment.
Fixed — kill_running() now sends a cancellation signal (oneshot) that pump_output races against output.message().await via tokio::select!. The cancel fires unconditionally on channel close/client disconnect, independent of whether the Kill RPC itself succeeds or times out, so a wedged guest process that never produces another Attach message no longer leaks the Attach stream + ChannelWriteHalf forever. On cancellation, pump_output also skips the trailing Wait RPC (blocks until the process actually exits — same class of hang, now avoided) since there's no channel left to report an exit status to.
Verified: cargo build/clippy -D warnings/fmt --check clean on both native and aarch64-unknown-linux-musl; full existing sessiond suite (20 tests, including client_disconnect_kills_running_execution which exercises this exact kill_running path) passes unchanged.
I did not add a new test that directly proves the task terminates rather than hanging — traced through why: this specific class of bug (a spawned task leaking forever vs. exiting) isn't observable through this test harness's SSH/gRPC-visible surface (channel close/disconnect already completed promptly before my fix too, since kill_running was already non-blocking/spawned — the bug was purely internal task lifecycle, and Rust/Tokio has no equivalent to Go's runtime.NumGoroutine() readily available here to assert on). The correctness argument instead rests on tokio::select!'s documented semantics: a cancelled branch's future is dropped and the loop exits deterministically the moment cancel_rx resolves, regardless of the other branch's state — same category of "verifiable by the primitive's contract, not needing empirical proof" as the try_send fix earlier in this PR.
| &self, | ||
| operation: &'static str, | ||
| path_and_query: String, | ||
| ) -> Result<T, HostedApiError> { |
There was a problem hiding this comment.
🧹 SSH bearer token sent as URL query param
validate_ssh_access issues GET /box/ssh-access/validate?token=; query strings are commonly captured by proxy/ingress/error-tracking access logs, unlike an Authorization header, widening token-leak surface — not exploitable from this diff alone.
There was a problem hiding this comment.
Same finding raised twice already on this PR (README.md and token.rs:265) — see: #965 (comment)
Short version: confirmed accurate, mirrors the legacy Go gateway's pre-existing contract (predates this PR), fixing it requires a coordinated change to the shared Hosted API contract across both gateways — out of scope here, tracked as a follow-up.
| go func() { | ||
| _ = client.Wait() | ||
| cancel() | ||
| }() | ||
| go func() { | ||
| <-runCtx.Done() | ||
| b.closeTransport() | ||
| }() | ||
|
|
||
| b.readLoop() | ||
|
|
||
| cancel() | ||
| b.teardownChannels() | ||
| _ = client.Close() // also closes the guest conn and stops client.Wait | ||
| b.wg.Wait() |
There was a problem hiding this comment.
🧹 Two goroutines untracked by wg despite doc claim
go func(){ client.Wait(); cancel() }() and go func(){ <-runCtx.Done(); b.closeTransport() }() are not wg.Add'd, so Run() can return before they finish, contradicting the "never leaks goroutines" comment at bridge.go:103; benign today since client.Close() makes Wait() return promptly.
There was a problem hiding this comment.
Fixed — Run()'s connection-lifecycle goroutines (client.Wait()->cancel(), runCtx.Done()->closeTransport()) are now tracked via b.wg.Add(2)/defer b.wg.Done(), so b.wg.Wait() actually waits for them alongside the per-channel goroutines — matching the "never leaks goroutines" doc comment for real now. Verified no deadlock: both goroutines terminate promptly regardless of when Wait() is called relative to them (client.Wait() returns once client.Close() runs a few lines earlier; runCtx.Done() unblocks once cancel() runs even earlier than that, and closeTransport() is idempotent via closeOnce either way). Full sessionbridge suite (15 tests) passes under go test -race -count=3.
nieyy
force-pushed
the
nieyy/russh
branch
2 times, most recently
from
3fe6c24 to
73bb3f4
Compare
| @@ -1,5 +1,5 @@ | |||
| #!/bin/bash | |||
| # Build boxlite-guest binary on macOS or Linux | |||
| # Build the guest-side binaries (boxlite-guest, boxlite-sessiond) on macOS or Linux | |||
There was a problem hiding this comment.
no need to separate this to two binaries, just combine them in boxlite-guest
There was a problem hiding this comment.
Single binary now (boxlite-guest runs the SSH service in-process).
nieyy
force-pushed
the
nieyy/russh
branch
2 times, most recently
from
e834755 to
d1407e0
Compare
| let addr = VsockAddr::new(VMADDR_CID_ANY, GUEST_SSH_PORT); | ||
| let listener = VsockListener::bind(addr).map_err(|e| { | ||
| BoxliteError::Internal(format!("bind SSH vsock port {GUEST_SSH_PORT}: {e}")) | ||
| })?; | ||
| info!(port = GUEST_SSH_PORT, "SSH listening on vsock"); | ||
|
|
||
| let mut backoff = backoff::ACCEPT_ERROR_BACKOFF_MIN; | ||
| loop { | ||
| match listener.accept().await { | ||
| Ok((stream, peer)) => { | ||
| backoff = backoff::ACCEPT_ERROR_BACKOFF_MIN; | ||
| info!(peer = %peer, "SSH vsock connection accepted"); | ||
| let ssh_server = ssh_server.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(e) = ssh_server.serve_stream(stream).await { | ||
| warn!(error = %e, "SSH connection ended with error"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
serve_vsock binds VMADDR_CID_ANY and auth_none in server.rs grants root to any vsock caller; peer CID is logged (line 74) but never checked against VMADDR_CID_HOST, so any process able to reach this vsock port gets unauthenticated root, relying solely on an unenforced network-topology assumption.
There was a problem hiding this comment.
Good catch — added a peer-CID check (only VMADDR_CID_HOST is accepted) before the connection reaches auth, on both the SSH listener and the main gRPC vsock listener.
| pub(super) async fn stdin(&mut self, data: &[u8]) { | ||
| let Some(running) = &self.running else { | ||
| debug!( | ||
| bytes = data.len(), | ||
| "dropping stdin received before execution start" | ||
| ); | ||
| return; | ||
| }; | ||
| let Some(stdin_tx) = &running.stdin_tx else { | ||
| debug!(execution_id = %running.execution_id, "dropping stdin received after EOF"); | ||
| return; | ||
| }; | ||
| let frame = ExecStdin { | ||
| execution_id: running.execution_id.clone(), | ||
| data: data.to_vec(), | ||
| close: false, | ||
| }; | ||
| match stdin_tx.try_send(frame) { | ||
| Ok(()) => {} | ||
| Err(mpsc::error::TrySendError::Full(_)) => { | ||
| warn!(execution_id = %running.execution_id, bytes = data.len(), | ||
| "stdin queue full — dropping frame (guest not draining stdin)"); | ||
| } | ||
| Err(mpsc::error::TrySendError::Closed(_)) => { | ||
| debug!(execution_id = %running.execution_id, "stdin stream already closed by guest"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
STDIN_QUEUE_DEPTH=32 with try_send drops frames on backpressure and only warn!-logs it while the SSH client's flow-control window reports success, causing silent stdin data loss under load (e.g. paste bursts).
There was a problem hiding this comment.
Fixed — the drop itself stays non-blocking (still try_send), but now the client gets a one-time stderr notice with the drop count once the execution ends, so it's no longer silent.
| async fn channel_open_session( | ||
| &mut self, | ||
| channel: Channel<Msg>, | ||
| reply: ChannelOpenHandle, | ||
| session: &mut Session, | ||
| ) -> Result<(), Self::Error> { | ||
| let mux = match self.ensure_mux().await { | ||
| Ok(mux) => mux, | ||
| Err(error) => { | ||
| self.fail_closed(&error); | ||
| reply | ||
| .reject(ChannelOpenFailure::AdministrativelyProhibited) | ||
| .await; | ||
| // The disconnect description is the user-visible error; it | ||
| // carries only the sanitized reason code. | ||
| let _ = session | ||
| .handle() | ||
| .disconnect( | ||
| Disconnect::ByApplication, | ||
| error.user_message(), | ||
| String::new(), | ||
| ) | ||
| .await; | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| let frame_channel = self.next_frame_channel; | ||
| self.next_frame_channel += 1; | ||
| let rx = match mux.register_channel(frame_channel) { | ||
| Ok(rx) => rx, | ||
| Err(_) => { | ||
| reply | ||
| .reject(ChannelOpenFailure::AdministrativelyProhibited) | ||
| .await; | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| let ssh_channel = channel.id(); | ||
| // Keep only the write half; incoming traffic arrives through the | ||
| // handler callbacks, and dropping the read half keeps russh from | ||
| // buffering messages nobody consumes. | ||
| let (_read_half, write_half) = channel.split(); | ||
| self.channels.insert( | ||
| ssh_channel, | ||
| ChannelBinding { | ||
| frame_channel, | ||
| open_state: None, | ||
| _write_half: write_half, | ||
| }, | ||
| ); | ||
| spawn_channel_forwarder(rx, session.handle(), ssh_channel, frame_channel); | ||
| reply.accept().await; | ||
| info!( | ||
| session = %self.session_id, | ||
| channel = frame_channel, | ||
| "session channel opened" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 No per-connection cap on opened SSH channels
channel_open_session has no upper bound on channels per authenticated connection; each spawns a forwarder task + mpsc queue, so one valid-token client can multiply tasks/queues against a single Runner (connection_limit only caps concurrent connections, not channels-per-connection).
There was a problem hiding this comment.
Added a per-connection channel cap (64) — channel_open_session now rejects with ResourceShortage once a connection is at the limit.
| /* | ||
| * Copyright 2025 BoxLite AI | ||
| * SPDX-License-Identifier: AGPL-3.0 | ||
| */ | ||
|
|
||
| import { BoxService } from './box.service' | ||
|
|
||
| // The ssh-access methods only touch sshAccessRepository, regionService and | ||
| // configService (plus findOneByIdOrName, spied per test); every other | ||
| // injected dependency is irrelevant. | ||
| function makeService() { | ||
| const sshAccessRepository = { | ||
| save: jest.fn().mockImplementation(async (entity: any) => entity), | ||
| delete: jest.fn().mockResolvedValue(undefined), | ||
| findOne: jest.fn(), | ||
| } as any | ||
| const regionService = { findOne: jest.fn().mockResolvedValue(null) } as any | ||
| const runnerService = { findOne: jest.fn() } as any | ||
| const configService = { getOrThrow: jest.fn() } as any | ||
| const noop = {} as any | ||
| const service = new BoxService( | ||
| noop, // boxRepository | ||
| noop, // runnerRepository | ||
| sshAccessRepository, // sshAccessRepository | ||
| runnerService, // runnerService | ||
| noop, // volumeService | ||
| configService, // configService | ||
| noop, // warmPoolService | ||
| noop, // eventEmitter | ||
| noop, // organizationService | ||
| noop, // runnerAdapterFactory | ||
| noop, // redisLockProvider | ||
| noop, // redis | ||
| regionService, // regionService | ||
| noop, // boxLookupCacheInvalidationService | ||
| noop, // boxActivityService | ||
| ) | ||
| return { service, sshAccessRepository, regionService, runnerService, configService } | ||
| } | ||
|
|
||
| const box = { id: 'box-1', region: 'us' } as any | ||
|
|
||
| describe('BoxService.createSshAccess', () => { | ||
| it('revokes existing SSH access for the box before saving the new token', async () => { | ||
| const { service, sshAccessRepository, configService } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
| configService.getOrThrow.mockReturnValue('gateway.example.com') | ||
|
|
||
| await service.createSshAccess('box-1') | ||
|
|
||
| expect(sshAccessRepository.delete).toHaveBeenCalledWith({ boxId: 'box-1' }) | ||
| // Revocation must land before the new token is persisted, otherwise the | ||
| // fresh token would be wiped along with the stale ones. | ||
| expect(sshAccessRepository.delete.mock.invocationCallOrder[0]).toBeLessThan( | ||
| sshAccessRepository.save.mock.invocationCallOrder[0], | ||
| ) | ||
| }) | ||
|
|
||
| it('generates a 32-char token without CLI-hostile _ or - characters', async () => { | ||
| const { service, sshAccessRepository, configService } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
| configService.getOrThrow.mockReturnValue('gateway.example.com') | ||
|
|
||
| await service.createSshAccess('box-1') | ||
|
|
||
| const saved = sshAccessRepository.save.mock.calls[0][0] | ||
| expect(saved.token).toHaveLength(32) | ||
| expect(saved.token).not.toMatch(/[_-]/) | ||
| }) | ||
|
|
||
| it('honors expiresInMinutes when computing expiresAt', async () => { | ||
| jest.useFakeTimers().setSystemTime(new Date('2026-01-01T00:00:00.000Z')) | ||
| try { | ||
| const { service, sshAccessRepository, configService } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
| configService.getOrThrow.mockReturnValue('gateway.example.com') | ||
|
|
||
| await service.createSshAccess('box-1', 15) | ||
|
|
||
| const saved = sshAccessRepository.save.mock.calls[0][0] | ||
| expect(saved.expiresAt).toEqual(new Date('2026-01-01T00:15:00.000Z')) | ||
| } finally { | ||
| jest.useRealTimers() | ||
| } | ||
| }) | ||
|
|
||
| it('builds sshCommand from the region SSH gateway url', async () => { | ||
| const { service, regionService } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
| regionService.findOne.mockResolvedValue({ sshGatewayUrl: 'ssh.example.com:2222' }) | ||
|
|
||
| const dto = await service.createSshAccess('box-1') | ||
|
|
||
| expect(regionService.findOne).toHaveBeenCalledWith('us', true) | ||
| expect(dto.sshCommand).toBe(`ssh -p 2222 ${dto.token}@ssh.example.com`) | ||
| }) | ||
|
|
||
| it('falls back to the configured gateway url when the region has none', async () => { | ||
| const { service, configService } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
| configService.getOrThrow.mockReturnValue('gateway.example.com') | ||
|
|
||
| const dto = await service.createSshAccess('box-1') | ||
|
|
||
| expect(configService.getOrThrow).toHaveBeenCalledWith('sshGateway.url') | ||
| expect(dto.sshCommand).toBe(`ssh ${dto.token}@gateway.example.com`) | ||
| }) | ||
| }) | ||
|
|
||
| describe('BoxService.validateSshAccess', () => { | ||
| const futureDate = () => new Date(Date.now() + 60 * 60 * 1000) | ||
|
|
||
| it('returns valid with boxId, unixUser and tokenId for a live token', async () => { | ||
| const { service, sshAccessRepository } = makeService() | ||
| sshAccessRepository.findOne.mockResolvedValue({ | ||
| id: 'access-1', | ||
| boxId: 'box-1', | ||
| token: 'live-token', | ||
| expiresAt: futureDate(), | ||
| box: { id: 'box-1', runnerId: null }, | ||
| }) | ||
|
|
||
| const result = await service.validateSshAccess('live-token') | ||
|
|
||
| expect(result).toEqual({ valid: true, boxId: 'box-1', unixUser: 'root', tokenId: 'access-1' }) | ||
| }) | ||
|
|
||
| it('returns the same contract when the box is assigned to a runner', async () => { | ||
| const { service, sshAccessRepository, runnerService } = makeService() | ||
| sshAccessRepository.findOne.mockResolvedValue({ | ||
| id: 'access-1', | ||
| boxId: 'box-1', | ||
| token: 'live-token', | ||
| expiresAt: futureDate(), | ||
| box: { id: 'box-1', runnerId: 'runner-1' }, | ||
| }) | ||
| runnerService.findOne.mockResolvedValue({ id: 'runner-1' }) | ||
|
|
||
| const result = await service.validateSshAccess('live-token') | ||
|
|
||
| expect(result).toEqual({ valid: true, boxId: 'box-1', unixUser: 'root', tokenId: 'access-1' }) | ||
| }) | ||
|
|
||
| it('returns invalid without unixUser/tokenId for an unknown token', async () => { | ||
| const { service, sshAccessRepository } = makeService() | ||
| sshAccessRepository.findOne.mockResolvedValue(null) | ||
|
|
||
| const result = await service.validateSshAccess('unknown-token') | ||
|
|
||
| // toStrictEqual also rejects extra keys set to undefined, so this proves | ||
| // the invalid path leaks no session details to the gateway. | ||
| expect(result).toStrictEqual({ valid: false, boxId: null }) | ||
| }) | ||
|
|
||
| it('returns invalid for an expired token', async () => { | ||
| const { service, sshAccessRepository } = makeService() | ||
| sshAccessRepository.findOne.mockResolvedValue({ | ||
| id: 'access-1', | ||
| boxId: 'box-1', | ||
| token: 'expired-token', | ||
| expiresAt: new Date(Date.now() - 1000), | ||
| box: { id: 'box-1', runnerId: null }, | ||
| }) | ||
|
|
||
| const result = await service.validateSshAccess('expired-token') | ||
|
|
||
| expect(result).toStrictEqual({ valid: false, boxId: null }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('BoxService.revokeSshAccess', () => { | ||
| it('deletes the specific token when one is given', async () => { | ||
| const { service, sshAccessRepository } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
|
|
||
| const revokedBox = await service.revokeSshAccess('box-1', 'token-1', 'org-1') | ||
|
|
||
| expect(sshAccessRepository.delete).toHaveBeenCalledWith({ boxId: 'box-1', token: 'token-1' }) | ||
| expect(revokedBox).toBe(box) | ||
| }) | ||
|
|
||
| it('deletes all SSH access for the box when no token is given', async () => { | ||
| const { service, sshAccessRepository } = makeService() | ||
| jest.spyOn(service, 'findOneByIdOrName').mockResolvedValue(box) | ||
|
|
||
| await service.revokeSshAccess('box-1') | ||
|
|
||
| expect(sshAccessRepository.delete).toHaveBeenCalledWith({ boxId: 'box-1' }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 No authorization-denied test cases for SSH access
Tests stub findOneByIdOrName directly, bypassing org-scoping guard logic; createSshAccess/revokeSshAccess have no cross-org or not-found coverage, only validateSshAccess's unknown/expired-token paths are tested.
There was a problem hiding this comment.
Added tests for cross-org and not-found box access on both createSshAccess and revokeSshAccess.
Provide standard SSH into a sandbox — interactive shell, non-PTY exec, binary-safe output, PTY resize, exit status — without depending on an sshd in the user image and without exposing VM transport details. Public path: ssh -p 2222 <token>@ssh.dev.boxlite.ai. - ssh-gateway: new russh public front door. The SSH username is an opaque access token; the gateway validates it, resolves box -> runner, and translates public SSH channels into BoxLite session frames. - session-frame: versioned Gateway-to-Runner frame protocol (Rust and Go codecs, shared spec in docs/architecture). - runner: HTTP-upgrade session bridge translating session frames to inner SSH channels and opening the guest stream via box.SSH(ctx). - boxlite: SSH as a first-class runtime capability — per-box ssh.sock bridged to guest vsock 2697, session readiness probe, raw SSH stream, and a stable session-error taxonomy. Exposed through the C ABI and the Go SDK. - guest: boxlite-guest serves SSH sessions in-process (russh) on a private vsock listener, delegating exec to its own gRPC service. - api: ssh-access validation now returns unixUser and tokenId. Wired into CI (unit tests + a real end-to-end SSH suite), local dev orchestration (apps/infra-local), and staging deploy code (apps/infra, not yet live in production).
2 participants
Summary
ssh -p 2222 <token>@ssh.dev.boxlite.ai, unchanged) backed by a BoxLite-owned SSH stack end to end: interactive shell, non-PTY exec, binary-safe output, PTY resize, and exit status — no dependency on an sshd in the user image, no VM transport details exposed.russh-based SSH Gateway (src/ssh-gateway) terminates public SSH, treats the SSH username as an opaque access token, validates it against the Hosted API, and translates public SSH channels into a versioned BoxLite session-frame protocol (src/session-frame,apps/runner/pkg/sessionframe, spec indocs/architecture/ssh-session-frame-protocol.md).apps/runner/pkg/sessionbridge) is the sole translation point between session frames and the inner SSH channels; it opens the guest stream viabox.SSH(ctx).boxlitecore exposes SSH as a first-class runtime capability: per-boxssh.sockbridged to guest vsock 2697,box.SessionReady(ctx, "ssh")readiness probe,box.SSH(ctx)raw stream, and a stableBoxSessionErrortaxonomy — exposed through the C ABI and the Go SDK.boxlite-guestruns the SSH session service in-process: arusshSSH server on a private vsock listener, delegating execution to its own gRPC Execution service. (Originally a separateboxlite-sessionddaemon; merged intoboxlite-guestper review feedback — one binary, no second guest-local delegation socket, and the SSH test suite now runs in CI for the first time as part of the guest crate.)ssh-accessvalidation now returnsunixUserandtokenId.debugfsexits 0 even when its commands fail, so an undersized image could silently produce a zero-filled injected binary that then got cached.debugfsstderr is now treated as fatal and the image is grown before injection.Test plan
cargo test -p boxlite-ssh-gateway(60 tests: unit + e2e + token validator)cargo test -p boxlite-session-framecargo test -p boxlite-guest(includes the in-process SSH session service; new CI job — this crate previously had zero test coverage in CI)BOXLITE_DEPS_STUB=1 cargo nextest run -p boxlite --no-default-features --lib(859 tests)cargo test -p boxlite --test session(real VM: SSH readiness probe reads theSSH-2.0-banner end to end)cd apps/runner && go test -race ./pkg/sessionframe/... ./pkg/sessionbridge/...go test -tags boxlite_dev ./pkg/api/controllers/...nx test api --testPathPatterns=box.service.ssh-access.spec(11 tests)Box.SSH()happy path (TestIntegrationSessionSSH)ssh -p 2222 <token>@hostthrough Gateway → Runner → VM (each leg is currently tested in isolation against a fake counterpart on the far side; needs a deployed environment)Summary by CodeRabbit
unixUserandtokenIdto SSH access validation responses (API + generated clients).ssh-status, plus internal upgrade-based session streaming.