Repository navigation
feat: add embedded SSH server - #1042
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe PR adds a Linux guest SSH service with certificate authorization, session and forwarding support, SFTP handling, typed workload execution, PTY mode propagation, process-group signaling, execution cleanup, guest-root sealing, host-key provisioning, and related integration tests. Embedded SSH execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
tester seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
boxlite-guest terminates SSH itself with russh, so a standard OpenSSH client reaches a box without an sshd package in the image. Scope is the guest only: the host APIs, gateway, CLI surface and docs are left for follow-ups. Authentication accepts one credential — a CA-signed OpenSSH user certificate naming this box. `none`, passwords and raw public keys are rejected outright, and the listener advertises publickey alone. Sessions run inside the container, not beside it. A reserved argv selects a typed workload that the zygote's executor validates and runs after libcontainer has applied the tenant's namespaces and credentials, so shell, exec, SFTP and socket forwarding all get the container's rootfs, uid and capability set with no helper binary in the image. Shell sessions exec the account's login shell with OpenSSH's argv[0] convention; PTY requests carry RFC 4254 terminal modes through to the pty. Those direct workloads enter the container by fork without execve, so their /proc/self/exe still names this binary. The guest root is remounted read-only once its tmpfs mounts are up, which closes that reopen without the memfd copy and re-exec a sealed self-image would cost. The server drives executions through the guest's own execution core in native types rather than a loopback gRPC client, so stdin and output are not protobuf-encoded to reach a struct in the same process. Ssh.Configure/Status/Disable let the host enable, rotate and disable the listener at runtime; rotation rejects certificates authenticated against a stale policy while established sessions drain. Two wire-contract additions come with it: KillRequest.process_group, so a session can tear down a shell's whole process group, and TtyConfig.modes for the terminal modes an SSH client sends. Existing hosts default both.
DorianZheng
force-pushed
the
agent/embed-ssh-server
branch
from
July 28, 2026 07:14
c77528c to
d006431
Compare
DorianZheng
marked this pull request as ready for review
July 28, 2026 08:09
📦 BoxLite review — looks good ·
|
DorianZheng
added
the
e2e-local
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
src/boxlite/tests/resize_tty.rs (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe final stdout drain is unbounded, unlike the readiness loop.
If
sttynever runs or stdout never closes, this loop blocks forever and the harness kills the test without ever printing the diagnostic on Line 62 — precisely the regression this test exists to catch. Thewait()below is bounded, but it is unreachable until the drain finishes.💚 Suggested change
- while let Some(chunk) = stream.next().await { - stdout.push_str(&chunk); - } + while let Ok(Some(chunk)) = tokio::time::timeout(Duration::from_secs(30), stream.next()).await { + stdout.push_str(&chunk); + }🤖 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/tests/resize_tty.rs` around lines 55 - 58, Bound the final stdout-drain loop in the resize TTY test so it cannot wait indefinitely when stty does not run or stdout remains open. Apply a timeout around the stream consumption after the readiness loop, while preserving the existing stdout collection and allowing execution.wait() to run afterward so the diagnostic remains reachable.src/guest/src/service/ssh/reverse_streamlocal.rs (2)
442-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the ingress address extraction.
local_addr().ok().unwrap_or_else(|| ...UNSPECIFIED...)fabricates a sentinel purely so the following two checks reject it. A directOk(SocketAddr::V4(addr))match expresses the same rejection without the placeholder.♻️ Suggested change
- let SocketAddr::V4(ingress_address) = ingress - .local_addr() - .ok() - .unwrap_or_else(|| SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, 0))) - else { + let Ok(SocketAddr::V4(ingress_address)) = ingress.local_addr() else { return false; };🤖 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/guest/src/service/ssh/reverse_streamlocal.rs` around lines 442 - 458, Update the ingress address extraction after TcpListener::local_addr in the reverse streamlocal setup to directly accept only Ok(SocketAddr::V4(addr)) and return false for errors or non-IPv4 addresses. Remove the fabricated UNSPECIFIED sentinel and retain the existing localhost and nonzero-port validation.
847-857: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a constant-time comparison for the ingress bearer token.
received != expected_tokenis a short-circuiting byte compare on a shared secret that an unprivileged loopback peer can probe repeatedly (no rate limit or attempt cap on the ingress accept path). A constant-time equality keeps this from being a byte-at-a-time oracle.♻️ Suggested change
- if received != expected_token { + let matched = received + .iter() + .zip(expected_token) + .fold(0_u8, |acc, (a, b)| acc | (a ^ b)) + == 0; + if !matched {🤖 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/guest/src/service/ssh/reverse_streamlocal.rs` around lines 847 - 857, Replace the short-circuiting comparison in the ingress token validation flow with the project’s available constant-time byte-equality helper, comparing received against expected_token while preserving the existing PermissionDenied error and successful Ok(stream) behavior.src/guest/src/service/ssh/sftp.rs (1)
915-922: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the OS error before building the status reply.
cvtpasseslast_os_error()intoio_statusand reads it again for.with_message, so if the status packet travels between the two calls the code and message can describe different errors. Use a singlestd::io::Errorvalue and format it once.🤖 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/guest/src/service/ssh/sftp.rs` around lines 915 - 922, Update cvt so it captures std::io::Error::last_os_error() once in the nonzero-result branch, then reuse that same error value for both io_status and the formatted message, ensuring the status code and message describe one OS error.src/guest/src/service/exec/state.rs (1)
448-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest relies on raw fd numbers, which the harness can recycle.
fd_is_open(task_raw_fd)/fd_is_open(tracked_fds)check fd numbers; another test thread in the same binary can allocate the same number between release and the assertion, turning a correct release into a spurious failure. The zygote test insrc/guest/src/container/zygote.rsavoids this by probing pipe identity (POLLERR / EOF on the retained peer) instead. Not blocking, just a flake source.🤖 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/guest/src/service/exec/state.rs` around lines 448 - 545, Update the release tests around state_with_tracked_handle and release_closes_handle_fds_and_aborts_forwarders_idempotently to verify descriptor closure through retained pipe-peer behavior or pipe identity, rather than raw fd-number availability via fd_is_open. Preserve the existing assertions for idempotent release, task cancellation, and retained state cleanup, and apply the same race-safe approach to tracked and task descriptors.src/guest/src/service/exec/timeout.rs (1)
37-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTimeout escalation never targets the process group, so background descendants can survive.
Both the SIGTERM and SIGKILL stages pass
process_group: false. The PR adds process-group signaling specifically to reach descendants that leader-only signals miss (see theprocess_group_kill_reaches_a_background_descendanttest), but the timeout watcher — the guest's own backstop for runaway executions — never uses it. A shell/exec workload that backgrounds a child (or traps SIGTERM) can leave that descendant running indefinitely even after the exec "times out" and is SIGKILLed.♻️ Suggested fix: escalate to the process group on timeout
- if !exec_state.kill(Signal::SIGTERM, false).await { + if !exec_state.kill(Signal::SIGTERM, true).await { // Process already exited on its own; nothing more to do. return; } @@ - if exec_state.kill(Signal::SIGKILL, false).await { + if exec_state.kill(Signal::SIGKILL, true).await {Please confirm whether container-scoped executions get equivalent cleanup via container teardown independent of this path — if not, this gap applies there too.
Also applies to: 54-54
🤖 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/guest/src/service/exec/timeout.rs` at line 37, Update the timeout escalation in the watcher around exec_state.kill so both the SIGTERM and SIGKILL calls target the process group by passing process_group: true. Preserve the existing escalation order and behavior while ensuring timed-out executions also terminate background descendants.src/guest/src/service/ssh/server.rs (1)
328-335: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
env_requestrejects updates to already-set variables once at the cap.At
MAX_ENV_VARS, re-sending an existing name fails even though the map size would not grow. Considerstate.env.len() >= MAX_ENV_VARS && !state.env.contains_key(variable_name), matching the guard already used inapply_pty_request.🤖 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/guest/src/service/ssh/server.rs` around lines 328 - 335, Update the environment-cap condition in env_request so it rejects only new variable names when state.env has reached MAX_ENV_VARS; allow updates to names already present in state.env while preserving the existing name, value-size, and NUL validation checks.src/guest/src/service/ssh/host_key.rs (1)
70-73: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCreate the host-key directory with restrictive mode.
create_dir_alluses the process umask (typically0o755). The key file itself is0600, so this isn't exploitable today, but tightening the directory to0o700matches the sshd convention and the0o077check enforced on the key.🛡️ Proposed hardening
- std::fs::create_dir_all(parent).map_err(HostKeyError::Io)?; + std::fs::create_dir_all(parent).map_err(HostKeyError::Io)?; + std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) + .map_err(HostKeyError::Io)?;(
use std::os::unix::fs::PermissionsExt;is already needed in scope.)🤖 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/guest/src/service/ssh/host_key.rs` around lines 70 - 73, Update the directory creation in the host-key path setup around path.parent() and create_dir_all to enforce restrictive 0o700 permissions, using PermissionsExt as indicated. Preserve the existing parent validation and HostKeyError::Io mapping while ensuring the resulting host-key directory is not group- or world-accessible.src/guest/src/service/ssh/mod.rs (1)
595-613: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePre-authentication budget is effectively 2×
AUTHENTICATION_TIMEOUT.
run_streamis wrapped in a 30s timeout, andmonitor_sessionthen starts a fresh 30s auth timer, so an idle peer can hold a connection permit for ~60s. Consider passing a deadline (Instant) through instead of two independent timers.Also applies to: 701-707
🤖 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/guest/src/service/ssh/mod.rs` around lines 595 - 613, Use one shared authentication deadline across the pre-authentication flow instead of starting independent timers around run_stream and monitor_session. Update the relevant handshake and monitor_session paths to derive remaining time from a single Instant deadline and pass that deadline through, ensuring an idle peer cannot exceed AUTHENTICATION_TIMEOUT overall while preserving shutdown and connection-policy cancellation behavior.src/guest/src/service/server.rs (1)
134-153: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRejected peers are logged at
warnper connection attempt.A tenant process can loop loopback vsock connects and flood guest logs. Consider rate-limiting or demoting to
debugafter the first rejection.🤖 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/guest/src/service/server.rs` around lines 134 - 153, Update the rejected-peer logging in the listener’s incoming connection filter to avoid emitting a warn record for every rejected connection attempt. Demote the per-peer rejection messages in the non-host and missing-address branches to debug-level logging, or reuse an existing rate-limited warning mechanism if available, while preserving rejection before tonic receives the stream.src/guest/src/service/ssh/bridge.rs (3)
741-746: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
tokio::spawn(async {})as a placeholder output task reads as a workaround.Making
spawn_execution_cleanuptakeOption<JoinHandle<()>>would express "no output task" directly.🤖 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/guest/src/service/ssh/bridge.rs` around lines 741 - 746, Update spawn_execution_cleanup and its callers to accept an Option<JoinHandle<()>>, allowing cleanup_failed_execution_start to pass None when no output task exists. Remove the placeholder tokio::spawn(async {}) handle while preserving the existing cleanup behavior for callers that provide an output task.
329-341: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking filesystem I/O runs on the runtime thread while the container mutex is held.
root_session_profiledoescanonicalize+readof the container's/etc/passwd(andsymlink_metadataper path component) synchronously. It happens on every channel open, under the per-container mutex that also serializes exec builds. Consider resolving it once per container and caching it, or moving the call intospawn_blocking.🤖 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/guest/src/service/ssh/bridge.rs` around lines 329 - 341, Update resolve_single_container_profile so the synchronous root_session_profile filesystem work does not run on the async runtime thread while holding the container mutex. Prefer resolving and caching the profile once per container; otherwise clone the container handle, release the mutex before calling root_session_profile, and perform that call via spawn_blocking while preserving the existing return and error behavior.
295-315: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTeardown always burns the full grace window, even when the process is already gone.
sleep_until(deadline)is unconditional, so a channel whose process exited on SIGHUP still holds the teardown task for the wholePROCESS_TERMINATION_GRACE. Racing the sleep against the execution's exit signal would release resources sooner.🤖 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/guest/src/service/ssh/bridge.rs` around lines 295 - 315, Update terminate_process_group to stop waiting once the execution’s exit signal is received, while retaining PROCESS_TERMINATION_GRACE as the maximum deadline before sending FORCED_TERMINATION_SIGNAL. Replace the unconditional sleep_until(deadline) with a race between the deadline and the execution exit notification, then only send the forced signal if the process has not already exited.src/guest/src/container/command.rs (1)
382-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard duplicates
BoxliteWorkloadExecutor::validate_spec, but the early check is worth keeping.Fails before the zygote round-trip; just keep the two error strings in sync (
src/guest/src/service/ssh/workload.rsLine 164-168 uses the same wording today).🤖 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/guest/src/container/command.rs` around lines 382 - 391, Keep the early PTY compatibility guard in the command path, but synchronize its BoxliteError::Config message with the validation message in BoxliteWorkloadExecutor::validate_spec. Ensure both checks use the exact same wording while preserving the existing pre-zygote return behavior.
🤖 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/guest/src/service/guest.rs`:
- Around line 100-108: Change the SSH quiesce handling around
self.ssh_manager.shutdown() so a failure is recorded and logged but does not
return early; allow the remaining teardown steps, including filesystem sync, to
run. Preserve the degraded quiesce error for reporting after Step 3 if the
function’s existing return flow supports it, while keeping successful shutdown
behavior unchanged.
In `@src/guest/src/service/ssh/bridge.rs`:
- Around line 317-327: Update ChannelBridge::drop and the terminate_running path
so cleanup does not panic when the Tokio runtime is shutting down: guard the
spawned teardown task and skip spawning if the runtime no longer accepts tasks.
Preserve normal channel-close behavior and ensure terminal shutdown still relies
on closing all sessions.
In `@src/guest/src/service/ssh/reverse_streamlocal.rs`:
- Around line 106-122: Update the reverse streamlocal bind flow around
UnixListener::bind to temporarily set the process umask to 0o177 before creating
the socket, then restore the previous umask immediately after bind, including
when bind fails. Keep the existing set_permissions normalization and
BoundSocket::capture behavior unchanged.
In `@src/guest/src/service/ssh/workload.rs`:
- Around line 663-689: Update the subprocess test around SUBPROCESS_CASE "exec"
to derive the expected directory from the process’s actual current working
directory after enter_root_home, rather than from inherited_cwd or the stale PWD
environment variable. Keep the shell command and output assertion unchanged; do
not modify enter_root_home unless explicitly choosing the separate
session-environment fix.
---
Nitpick comments:
In `@src/boxlite/tests/resize_tty.rs`:
- Around line 55-58: Bound the final stdout-drain loop in the resize TTY test so
it cannot wait indefinitely when stty does not run or stdout remains open. Apply
a timeout around the stream consumption after the readiness loop, while
preserving the existing stdout collection and allowing execution.wait() to run
afterward so the diagnostic remains reachable.
In `@src/guest/src/container/command.rs`:
- Around line 382-391: Keep the early PTY compatibility guard in the command
path, but synchronize its BoxliteError::Config message with the validation
message in BoxliteWorkloadExecutor::validate_spec. Ensure both checks use the
exact same wording while preserving the existing pre-zygote return behavior.
In `@src/guest/src/service/exec/state.rs`:
- Around line 448-545: Update the release tests around state_with_tracked_handle
and release_closes_handle_fds_and_aborts_forwarders_idempotently to verify
descriptor closure through retained pipe-peer behavior or pipe identity, rather
than raw fd-number availability via fd_is_open. Preserve the existing assertions
for idempotent release, task cancellation, and retained state cleanup, and apply
the same race-safe approach to tracked and task descriptors.
In `@src/guest/src/service/exec/timeout.rs`:
- Line 37: Update the timeout escalation in the watcher around exec_state.kill
so both the SIGTERM and SIGKILL calls target the process group by passing
process_group: true. Preserve the existing escalation order and behavior while
ensuring timed-out executions also terminate background descendants.
In `@src/guest/src/service/server.rs`:
- Around line 134-153: Update the rejected-peer logging in the listener’s
incoming connection filter to avoid emitting a warn record for every rejected
connection attempt. Demote the per-peer rejection messages in the non-host and
missing-address branches to debug-level logging, or reuse an existing
rate-limited warning mechanism if available, while preserving rejection before
tonic receives the stream.
In `@src/guest/src/service/ssh/bridge.rs`:
- Around line 741-746: Update spawn_execution_cleanup and its callers to accept
an Option<JoinHandle<()>>, allowing cleanup_failed_execution_start to pass None
when no output task exists. Remove the placeholder tokio::spawn(async {}) handle
while preserving the existing cleanup behavior for callers that provide an
output task.
- Around line 329-341: Update resolve_single_container_profile so the
synchronous root_session_profile filesystem work does not run on the async
runtime thread while holding the container mutex. Prefer resolving and caching
the profile once per container; otherwise clone the container handle, release
the mutex before calling root_session_profile, and perform that call via
spawn_blocking while preserving the existing return and error behavior.
- Around line 295-315: Update terminate_process_group to stop waiting once the
execution’s exit signal is received, while retaining PROCESS_TERMINATION_GRACE
as the maximum deadline before sending FORCED_TERMINATION_SIGNAL. Replace the
unconditional sleep_until(deadline) with a race between the deadline and the
execution exit notification, then only send the forced signal if the process has
not already exited.
In `@src/guest/src/service/ssh/host_key.rs`:
- Around line 70-73: Update the directory creation in the host-key path setup
around path.parent() and create_dir_all to enforce restrictive 0o700
permissions, using PermissionsExt as indicated. Preserve the existing parent
validation and HostKeyError::Io mapping while ensuring the resulting host-key
directory is not group- or world-accessible.
In `@src/guest/src/service/ssh/mod.rs`:
- Around line 595-613: Use one shared authentication deadline across the
pre-authentication flow instead of starting independent timers around run_stream
and monitor_session. Update the relevant handshake and monitor_session paths to
derive remaining time from a single Instant deadline and pass that deadline
through, ensuring an idle peer cannot exceed AUTHENTICATION_TIMEOUT overall
while preserving shutdown and connection-policy cancellation behavior.
In `@src/guest/src/service/ssh/reverse_streamlocal.rs`:
- Around line 442-458: Update the ingress address extraction after
TcpListener::local_addr in the reverse streamlocal setup to directly accept only
Ok(SocketAddr::V4(addr)) and return false for errors or non-IPv4 addresses.
Remove the fabricated UNSPECIFIED sentinel and retain the existing localhost and
nonzero-port validation.
- Around line 847-857: Replace the short-circuiting comparison in the ingress
token validation flow with the project’s available constant-time byte-equality
helper, comparing received against expected_token while preserving the existing
PermissionDenied error and successful Ok(stream) behavior.
In `@src/guest/src/service/ssh/server.rs`:
- Around line 328-335: Update the environment-cap condition in env_request so it
rejects only new variable names when state.env has reached MAX_ENV_VARS; allow
updates to names already present in state.env while preserving the existing
name, value-size, and NUL validation checks.
In `@src/guest/src/service/ssh/sftp.rs`:
- Around line 915-922: Update cvt so it captures std::io::Error::last_os_error()
once in the nonzero-result branch, then reuse that same error value for both
io_status and the formatted message, ensuring the status code and message
describe one OS error.
🪄 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: 69805489-9590-4fa3-aa46-d8af62848243
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
make/test.mksrc/boxlite/src/portal/interfaces/exec.rssrc/boxlite/tests/resize_tty.rssrc/boxlite/tests/security_enforcement.rssrc/guest/Cargo.tomlsrc/guest/src/container/command.rssrc/guest/src/container/lifecycle.rssrc/guest/src/container/mod.rssrc/guest/src/container/user_profile.rssrc/guest/src/container/zygote.rssrc/guest/src/main.rssrc/guest/src/mounts.rssrc/guest/src/reaper.rssrc/guest/src/service/exec/error.rssrc/guest/src/service/exec/exec_handle.rssrc/guest/src/service/exec/executor.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rssrc/guest/src/service/exec/tty.rssrc/guest/src/service/guest.rssrc/guest/src/service/mod.rssrc/guest/src/service/server.rssrc/guest/src/service/ssh/auth.rssrc/guest/src/service/ssh/backoff.rssrc/guest/src/service/ssh/bridge.rssrc/guest/src/service/ssh/control.rssrc/guest/src/service/ssh/forward.rssrc/guest/src/service/ssh/host_key.rssrc/guest/src/service/ssh/limits.rssrc/guest/src/service/ssh/mod.rssrc/guest/src/service/ssh/reverse_streamlocal.rssrc/guest/src/service/ssh/server.rssrc/guest/src/service/ssh/session.rssrc/guest/src/service/ssh/sftp.rssrc/guest/src/service/ssh/streamlocal.rssrc/guest/src/service/ssh/workload.rssrc/shared/proto/boxlite/v1/service.protosrc/shared/src/lib.rs
Comment on lines
+317
to
+327
| impl Drop for ChannelBridge { | ||
| fn drop(&mut self) { | ||
| // Transport loss may drop the russh handler without delivering a | ||
| // channel-close callback. Keep SSH-owned processes tied to the channel | ||
| // in that path as well; terminate_running takes the cancel sender so a | ||
| // normal channel-close does not start teardown twice. | ||
| if self.output_cancel.is_some() { | ||
| self.terminate_running(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
tokio::spawn in Drop panics if the bridge is dropped while the runtime is shutting down.
Transport loss during guest shutdown drops handlers from the runtime shutdown path, where spawn panics with "A Tokio 1.x context was found, but it is being shut down". Since terminal shutdown closes all sessions anyway, a guarded spawn is enough.
🛡️ Proposed guard
fn drop(&mut self) {
- if self.output_cancel.is_some() {
+ if self.output_cancel.is_some() && tokio::runtime::Handle::try_current().is_ok() {
self.terminate_running();
+ } else if let Some(cancel) = self.output_cancel.take() {
+ let _ = cancel.send(());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl Drop for ChannelBridge { | |
| fn drop(&mut self) { | |
| // Transport loss may drop the russh handler without delivering a | |
| // channel-close callback. Keep SSH-owned processes tied to the channel | |
| // in that path as well; terminate_running takes the cancel sender so a | |
| // normal channel-close does not start teardown twice. | |
| if self.output_cancel.is_some() { | |
| self.terminate_running(); | |
| } | |
| } | |
| } | |
| impl Drop for ChannelBridge { | |
| fn drop(&mut self) { | |
| // Transport loss may drop the russh handler without delivering a | |
| // channel-close callback. Keep SSH-owned processes tied to the channel | |
| // in that path as well; terminate_running takes the cancel sender so a | |
| // normal channel-close does not start teardown twice. | |
| if self.output_cancel.is_some() && tokio::runtime::Handle::try_current().is_ok() { | |
| self.terminate_running(); | |
| } else if let Some(cancel) = self.output_cancel.take() { | |
| let _ = cancel.send(()); | |
| } | |
| } | |
| } |
🤖 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/guest/src/service/ssh/bridge.rs` around lines 317 - 327, Update
ChannelBridge::drop and the terminate_running path so cleanup does not panic
when the Tokio runtime is shutting down: guard the spawned teardown task and
skip spawning if the runtime no longer accepts tasks. Preserve normal
channel-close behavior and ensure terminal shutdown still relies on closing all
sessions.
Comment on lines
+663
to
+689
| let command = r#"printf '\036%s\0%s\037' "$PWD" 'literal spaces ; $(not reparsed)'"#; | ||
| let output = Command::new(std::env::current_exe().unwrap()) | ||
| .args([ | ||
| "--exact", | ||
| SUBPROCESS_TEST, | ||
| "--nocapture", | ||
| "--test-threads=1", | ||
| ]) | ||
| .env(SUBPROCESS_CASE, "exec") | ||
| .env(SUBPROCESS_COMMAND, command) | ||
| .current_dir(inherited_cwd.path()) | ||
| .output() | ||
| .unwrap(); | ||
| assert!( | ||
| output.status.success(), | ||
| "{}", | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
| let expected = format!( | ||
| "\u{1e}{}\0literal spaces ; $(not reparsed)\u{1f}", | ||
| expected_home.display() | ||
| ); | ||
| assert!(output | ||
| .stdout | ||
| .windows(expected.len()) | ||
| .any(|window| window == expected.as_bytes())); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Failing test: $PWD is not updated by set_current_dir, so the assertion compares against the inherited tempdir.
enter_root_home uses std::env::set_current_dir and exports only HOME; the PWD variable the subprocess inherits still points at inherited_cwd (set via .current_dir(...)). A non-interactive sh -c (dash and friends) does not recompute PWD, so the printed value is the tempdir, not the resolved root home — which matches the CI failure on both arches. Query the real cwd instead.
🐛 Proposed fix
- let command = r#"printf '\036%s\0%s\037' "$PWD" 'literal spaces ; $(not reparsed)'"#;
+ let command = r#"printf '\036%s\0%s\037' "$(pwd)" 'literal spaces ; $(not reparsed)'"#;If the intent is that SSH sessions also see a correct PWD, the durable fix is to export it alongside HOME in enter_root_home (src/guest/src/service/ssh/session.rs Line 53-57) and keep the assertion as-is.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let command = r#"printf '\036%s\0%s\037' "$PWD" 'literal spaces ; $(not reparsed)'"#; | |
| let output = Command::new(std::env::current_exe().unwrap()) | |
| .args([ | |
| "--exact", | |
| SUBPROCESS_TEST, | |
| "--nocapture", | |
| "--test-threads=1", | |
| ]) | |
| .env(SUBPROCESS_CASE, "exec") | |
| .env(SUBPROCESS_COMMAND, command) | |
| .current_dir(inherited_cwd.path()) | |
| .output() | |
| .unwrap(); | |
| assert!( | |
| output.status.success(), | |
| "{}", | |
| String::from_utf8_lossy(&output.stderr) | |
| ); | |
| let expected = format!( | |
| "\u{1e}{}\0literal spaces ; $(not reparsed)\u{1f}", | |
| expected_home.display() | |
| ); | |
| assert!(output | |
| .stdout | |
| .windows(expected.len()) | |
| .any(|window| window == expected.as_bytes())); | |
| } | |
| let command = r#"printf '\036%s\0%s\037' "$(pwd)" 'literal spaces ; $(not reparsed)'"#; | |
| let output = Command::new(std::env::current_exe().unwrap()) | |
| .args([ | |
| "--exact", | |
| SUBPROCESS_TEST, | |
| "--nocapture", | |
| "--test-threads=1", | |
| ]) | |
| .env(SUBPROCESS_CASE, "exec") | |
| .env(SUBPROCESS_COMMAND, command) | |
| .current_dir(inherited_cwd.path()) | |
| .output() | |
| .unwrap(); | |
| assert!( | |
| output.status.success(), | |
| "{}", | |
| String::from_utf8_lossy(&output.stderr) | |
| ); | |
| let expected = format!( | |
| "\u{1e}{}\0literal spaces ; $(not reparsed)\u{1f}", | |
| expected_home.display() | |
| ); | |
| assert!(output | |
| .stdout | |
| .windows(expected.len()) | |
| .any(|window| window == expected.as_bytes())); | |
| } |
🧰 Tools
🪛 GitHub Actions: Test / Rust Tests (linux-arm64-gnu)
[error] 685-685: Test failed in boxlite_guest::service::ssh::workload::tests::exec_workload_preserves_one_command_argument_and_root_home. Panic: assertion failed: output.stdout.windows(expected.len()).any(|window| window == expected.as_bytes()).
🪛 GitHub Actions: Test / Rust Tests (linux-x64-gnu)
[error] 685-685: Test failed: assertion failed in exec_workload_preserves_one_command_argument_and_root_home. Assertion: output.stdout.windows(expected.len()).any(|window| window == expected.as_bytes()).
🤖 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/guest/src/service/ssh/workload.rs` around lines 663 - 689, Update the
subprocess test around SUBPROCESS_CASE "exec" to derive the expected directory
from the process’s actual current working directory after enter_root_home,
rather than from inherited_cwd or the stale PWD environment variable. Keep the
shell command and output assertion unchanged; do not modify enter_root_home
unless explicitly choosing the separate session-environment fix.
Source: Pipeline failures
added 4 commits
July 28, 2026 16:39
Two workload subprocess tests encoded assumptions about the runner, and the guest's new C dependency needs a cross toolchain the macOS lint job lacks. selected_home mirrored session::enter_root_home's fallback with is_dir(), but root's home is a directory for everyone and traversable only by root — an unprivileged run legitimately lands on / while the test still expected /root. It now asks for search permission, which is what set_current_dir needs. This is the failure CI reported. The descriptor-boundary test dup2'd onto a fixed fd 4096. dup2 rejects a target at or above RLIMIT_NOFILE with EBADF, so the call fails before close_inherited_fds runs wherever the soft limit is lower; it now picks the highest descriptor the process may actually hold. CI cancelled after the first failure, so whether it reached this test is unknown — the fix is for robustness, not a diagnosed CI failure. Only the macOS branch of `make clippy` cross-compiles boxlite-guest to a linux-musl target, and the guest links C since russh brought in ring. That job now installs the same bottled cross toolchain and linker config scripts/setup/setup-macos.sh gives developers. Linux clippy builds the guest for its host target and needs nothing new.
The Shutdown RPC returned as soon as the SSH quiesce timed out — before containers were stopped, init exit records were written, or filesystems were synced. That left COW disks inconsistent on the next start and the box's exit status unrecorded, and nothing else picks the teardown back up: `shutting_down` has already told the reaper to stand down. The quiesce only buys ordering against session handlers registering new executions, so a stuck session now logs and the teardown continues. The failure is still reported, after the remaining steps have run.
`bind` creates a pathname socket with `0777 & ~umask`, and `connect(2)` on one needs only write permission, so the chmod that followed left a window in which any container process could reach the listener. The socket may sit anywhere the target validator allows, including a world-writable directory, so the umask has to be narrow across the bind.
The root account's home and shell were resolved from the guest, against the container's mounted rootfs. Reaching into another mount namespace by path meant hand-rolling a chroot-like symlink walker, and its result was then discarded: every session workload re-resolves the profile inside the container and chdirs there itself, and the login shell never launched anything — the program comes from the workload placeholder. Resolution now happens once, where passwd is actually readable, through the same `getpwuid_r` libcontainer uses to seed HOME. The shell fallback list goes with it: OpenSSH substitutes /bin/sh only for an empty passwd field and otherwise execs what passwd names, so a shell an image does not ship now fails visibly instead of silently becoming a different shell. HOME and SHELL are dropped from the exec request rather than overwritten, since neither has a correct value outside the container.
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
russhSSH endpoint inboxlite-guestwith CA-signed certificate authenticationGuest.Init, plus host APIs and a stdio ProxyCommand transportWhy
SSH access was coupled to gateway and image assumptions and could not be safely reconfigured at runtime. The guest now owns SSH protocol termination while the host owns mutable policy and credential reconciliation.
User impact
Standard OpenSSH clients can connect without an
sshdpackage in the image. CA credentials can rotate without restarting the guest: existing authenticated sessions drain normally, while terminal shutdown closes all sessions. Password authentication, raw public-key authentication, agent forwarding, and unsafe forwarding modes remain disabled.Validation
make test:unit:guest— 210 unit tests plus 7 SSH integration tests passed on LinuxBOXLITE_DEPS_STUB=1 make test:unit:rust FILTER=ssh— 6 tests passedBOXLITE_DEPS_STUB=1 make test:integration:cli FILTER=network_tunnel_accepts_stdio_proxy_mode SETUP_DONE=1make clippymake guestmake fmt:check:rustgit diff --checkA full
make fmt:checkwas also attempted, but the apps dependency bootstrap failed while building native sqlite3 before formatting ran.Summary by CodeRabbit