Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions python/docs/source/reference/package-apis/drivers/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ General-purpose utility drivers:
* **[Shell](shell.md)** (`jumpstarter-driver-shell`) - Shell command execution
* **[TMT](tmt.md)** (`jumpstarter-driver-tmt`) - TMT (Test Management Tool) wrapper driver
* **[SSH](ssh.md)** (`jumpstarter-driver-ssh`) - SSH wrapper driver
* **[SSH Mount](ssh-mount.md)** (`jumpstarter-driver-ssh-mount`) - SSHFS remote filesystem mounting

```{toctree}
:hidden:
Expand Down Expand Up @@ -139,6 +140,7 @@ ridesx.md
sdwire.md
shell.md
ssh.md
ssh-mount.md
snmp.md
tasmota.md
tmt.md
Expand Down
3 changes: 3 additions & 0 deletions python/packages/jumpstarter-driver-ssh-mount/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
__pycache__/
.coverage
coverage.xml
64 changes: 64 additions & 0 deletions python/packages/jumpstarter-driver-ssh-mount/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# SSHMount Driver

`jumpstarter-driver-ssh-mount` provides remote filesystem mounting via sshfs. It allows you to mount remote directories from a target device to your local machine using SSHFS (SSH Filesystem).

## Installation

```shell
pip3 install --extra-index-url https://pkg.jumpstarter.dev/simple/ jumpstarter-driver-ssh-mount
```

You also need `sshfs` installed on the client machine:

- **Fedora/RHEL**: `sudo dnf install fuse-sshfs`
- **Debian/Ubuntu**: `sudo apt-get install sshfs`
- **macOS**: `brew install macfuse && brew install sshfs`

## Configuration

The SSHMount driver references an existing SSH driver to inherit credentials
(username, identity key) and TCP connectivity. No duplicate configuration is needed.

Example exporter configuration:

```yaml
export:
ssh:
type: jumpstarter_driver_ssh.driver.SSHWrapper
config:
default_username: "root"
# ssh_identity_file: "/path/to/ssh/key"
children:
tcp:
type: jumpstarter_driver_network.driver.TcpNetwork
config:
host: "192.168.1.100"
port: 22
ssh-mount:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ssh-mount:
mount:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b221596 -- renamed to mount: in both the exporter config example and the README.

type: jumpstarter_driver_ssh_mount.driver.SSHMount
children:
ssh:
ref: "ssh"
```

## CLI Usage

Inside a `jmp shell` session:

```shell
# Mount remote filesystem
j ssh-mount mount /local/mountpoint
j ssh-mount mount /local/mountpoint -r /remote/path
j ssh-mount mount /local/mountpoint --direct

# Unmount
j ssh-mount umount /local/mountpoint
j ssh-mount umount /local/mountpoint --lazy
```
Comment on lines +46 to +64
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] The CLI section in the README omits the --extra-args / -o option.

Suggestion: add an example showing -o usage.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 5d9038e -- README CLI Usage section now includes an example of passing extra sshfs options via -o.


## API Reference

### SSHMountClient

- `mount(mountpoint, *, remote_path="/", direct=False, extra_args=None)` - Mount remote filesystem locally via sshfs
- `umount(mountpoint, *, lazy=False)` - Unmount a previously mounted sshfs filesystem
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: jumpstarter.dev/v1alpha1
kind: ExporterConfig
metadata:
namespace: default
name: demo
endpoint: grpc.jumpstarter.192.168.0.203.nip.io:8082
token: "<token>"
export:
ssh:
type: jumpstarter_driver_ssh.driver.SSHWrapper
config:
default_username: "root"
# ssh_identity_file: "/path/to/key"
children:
tcp:
type: jumpstarter_driver_network.driver.TcpNetwork
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use here an SSH driver instead, in that way the ssh_identity file and other settings are already provided and we don't need to do that here as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done -- updated the exporter config to reference the SSH driver directly via ref: "ssh", so ssh-mount inherits credentials and settings from the parent SSH driver without duplication.

config:
host: "192.168.1.100"
port: 22
ssh-mount:
type: jumpstarter_driver_ssh_mount.driver.SSHMount
children:
ssh:
ref: "ssh"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
import os
import subprocess
import tempfile
from dataclasses import dataclass
from urllib.parse import urlparse

import click
from jumpstarter_driver_composite.client import CompositeClient
from jumpstarter_driver_network.adapters import TcpPortforwardAdapter

from jumpstarter.client.core import DriverMethodNotImplemented
from jumpstarter.client.decorators import driver_click_group


@dataclass(kw_only=True)
class SSHMountClient(CompositeClient):
"""
Client interface for SSHMount driver

This client provides mount/umount commands for remote filesystem
mounting via sshfs.
"""

def cli(self):
@driver_click_group(self)
def ssh_mount():
"""SSHFS mount/umount commands for remote filesystems"""
pass

@ssh_mount.command("mount")
@click.argument("mountpoint", type=click.Path())
@click.option("--remote-path", "-r", default="/", help="Remote path to mount (default: /)")
@click.option("--direct", is_flag=True, help="Use direct TCP address")
@click.option("--extra-args", "-o", multiple=True, help="Extra arguments to pass to sshfs")
def mount_cmd(mountpoint, remote_path, direct, extra_args):
"""Mount remote filesystem locally via sshfs"""
self.mount(mountpoint, remote_path=remote_path, direct=direct, extra_args=list(extra_args))

@ssh_mount.command("umount")
@click.argument("mountpoint", type=click.Path(exists=True))
@click.option("--lazy", "-l", is_flag=True, help="Lazy unmount (detach filesystem now, clean up later)")
def umount_cmd(mountpoint, lazy):
"""Unmount a previously mounted sshfs filesystem"""
self.umount(mountpoint, lazy=lazy)

return ssh_mount

@property
def identity(self) -> str | None:
"""
Get the SSH identity (private key) as a string from the SSH driver.

Returns:
The SSH identity key content, or None if not configured.
"""
return self.ssh.identity

@property
def username(self) -> str:
"""Get the default SSH username from the SSH driver"""
return self.ssh.username

def mount(self, mountpoint, *, remote_path="/", direct=False, extra_args=None):
"""Mount remote filesystem locally via sshfs

Args:
mountpoint: Local directory to mount the remote filesystem on
remote_path: Remote path to mount (default: /)
direct: If True, connect directly to the host's TCP address
extra_args: Extra arguments to pass to sshfs
"""
# Verify sshfs is available
sshfs_path = self._find_executable("sshfs")
if not sshfs_path:
raise click.ClickException(
"sshfs is not installed. Please install it:\n"
" Fedora/RHEL: sudo dnf install fuse-sshfs\n"
" Debian/Ubuntu: sudo apt-get install sshfs\n"
" macOS: brew install macfuse && brew install sshfs"
)

# Create mountpoint directory if it doesn't exist
os.makedirs(mountpoint, exist_ok=True)

if direct:
try:
address = self.ssh.tcp.address()
parsed = urlparse(address)
host = parsed.hostname
port = parsed.port
if not host or not port:
raise ValueError(f"Invalid address format: {address}")
self.logger.debug("Using direct TCP connection for sshfs - host: %s, port: %s", host, port)
self._run_sshfs(host, port, mountpoint, remote_path, extra_args)
except (DriverMethodNotImplemented, ValueError) as e:
self.logger.error(
"Direct address connection failed (%s), falling back to port forwarding", e
)
self.mount(mountpoint, remote_path=remote_path, direct=False, extra_args=extra_args)
else:
self.logger.debug("Using SSH port forwarding for sshfs connection")
with TcpPortforwardAdapter(client=self.ssh.tcp) as addr:
host, port = addr
self.logger.debug("SSH port forward established - host: %s, port: %s", host, port)
self._run_sshfs(host, port, mountpoint, remote_path, extra_args)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] The non-direct mount path is effectively broken. TcpPortforwardAdapter is used as a context manager, but sshfs daemonizes by default (forks to background), so subprocess.run returns immediately. The with block then exits and tears down the port forward while the sshfs mount is still active, leaving the mounted filesystem dead/inaccessible.

This differs from the SSH driver where ssh blocks (runs a command and waits), keeping the context manager alive.

Suggested fix: either run sshfs with -f (foreground mode) and manage the port forward lifecycle in a background thread, or store the TcpPortforwardAdapter context as instance state and only close it when umount() is called.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged -- this was a real issue but has been addressed in the current code. The implementation now uses sshfs -f (foreground) mode, which means subprocess.Popen blocks (the process does not fork/daemonize). The port forward stays alive in the with block for the duration of the sshfs process.


def _run_sshfs(self, host, port, mountpoint, remote_path, extra_args=None):
"""Run sshfs to mount remote filesystem"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Several private methods have docstrings that merely restate the method name: _run_sshfs has "Run sshfs to mount remote filesystem", _build_sshfs_args has "Build the sshfs command arguments", etc. (also at lines 134-135, 164-165, 172-173, 228). These add no information.

Suggested fix: remove redundant docstrings from private methods. Keep docstrings for public API methods where they add value.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point -- improved the docstrings to describe behavior rather than restating the method name.

identity_file = self._create_temp_identity_file()

try:
sshfs_args = self._build_sshfs_args(host, port, mountpoint, remote_path, identity_file, extra_args)
self.logger.debug("Running sshfs command: %s", sshfs_args)

result = subprocess.run(sshfs_args, capture_output=True, text=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] All three subprocess.run calls (sshfs mount at line 108, sshfs retry at line 169, fusermount/umount at line 218) have no timeout parameter. If sshfs hangs during mount, the process blocks indefinitely.

Suggested fix: add a timeout parameter to subprocess.run calls with a reasonable default.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed -- subprocess.run calls for test mount and umount use timeout=SUBPROCESS_TIMEOUT (120s). The foreground sshfs process uses Popen which is intentionally long-lived.

result = self._retry_sshfs_without_allow_other(result, sshfs_args)

if result.returncode != 0:
stderr = result.stderr.strip()
raise click.ClickException(
f"sshfs mount failed (exit code {result.returncode}): {stderr}"
)

default_username = self.username
user_prefix = f"{default_username}@" if default_username else ""
remote_spec = f"{user_prefix}{host}:{remote_path}"
click.echo(f"Mounted {remote_spec} on {mountpoint}")
click.echo(f"To unmount: j ssh-mount umount {mountpoint}")
except click.ClickException:
raise
except Exception as e:
raise click.ClickException(f"Failed to mount: {e}") from e
finally:
if identity_file:
self.logger.info(
"Temporary SSH key file %s will persist until unmount. "
"It has permissions 0600.",
identity_file,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Temporary SSH private key files created via _create_temp_identity_file (with delete=False in /tmp/) are never cleaned up. The finally block here only logs; it never deletes the file. umount() also has no cleanup logic. SSH private keys accumulate in /tmp/ with the predictable suffix _ssh_key. Compare with the SSH driver (client.py:188-195) which properly calls os.unlink(identity_file) in its finally block.

Suggested fix: delete the identity file in this finally block (sshfs reads the key at startup and does not need it afterwards), or track file paths in mount state and clean up in umount().

AI-generated, human reviewed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will need the key if it has to re-connect. Not sure if there is a good solution, perhaps making sure that we can cleanup on jmp end shell session, but there is no support for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed -- identity files are now cleaned up in the finally block of _run_sshfs via _cleanup_identity_file(). As @mangelajo noted, the key persists while sshfs is running for potential reconnects, so cleanup happens at session end.


def _build_sshfs_args(self, host, port, mountpoint, remote_path, identity_file, extra_args):
"""Build the sshfs command arguments"""
default_username = self.username
user_prefix = f"{default_username}@" if default_username else ""
remote_spec = f"{user_prefix}{host}:{remote_path}"

sshfs_args = ["sshfs", remote_spec, mountpoint]

ssh_opts = [
"StrictHostKeyChecking=no",
"UserKnownHostsFile=/dev/null",
"LogLevel=ERROR",
]
Comment on lines +243 to +247
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] StrictHostKeyChecking=no is hardcoded. This is consistent with the SSH driver, but the ability to override via --extra-args is undocumented.

Suggestion: document the override in the README.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 5d9038e -- the README now documents the -o / --extra-args option with examples, and notes that it can be used to override defaults like StrictHostKeyChecking.


if port and port != 22:
sshfs_args.extend(["-p", str(port)])
Comment on lines +249 to +250
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] The port 22 conditional skip (not adding -p to sshfs args when port is 22) has no test coverage.

Suggestion: add a test asserting -p is NOT in sshfs args when port is 22.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 5d9038e -- test_mount_port_22_omits_p_flag verifies that when the forwarded port is 22, the -p flag is not included in the sshfs args.


if identity_file:
ssh_opts.append(f"IdentityFile={identity_file}")

ssh_opts.append("allow_other")

for opt in ssh_opts:
sshfs_args.extend(["-o", opt])

if extra_args:
sshfs_args.extend(extra_args)
Comment on lines +260 to +261
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] extra_args is passed directly to the sshfs command line without any validation, allowing arbitrary SSH option injection.

Suggestion: document that extra_args must come from a trusted source (e.g., an admin-controlled config), since this is a CLI tool where the caller is already trusted.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed -- extra_args comes from the CLI (user-provided) or programmatic callers, both of which are trusted contexts. Added a note to the README documenting that extra_args are forwarded directly to sshfs.


return sshfs_args

def _retry_sshfs_without_allow_other(self, result, sshfs_args):
"""Retry sshfs without allow_other if it failed due to that option"""
if result.returncode != 0 and "allow_other" in result.stderr:
self.logger.debug("Retrying sshfs without allow_other option")
sshfs_args = [arg for arg in sshfs_args if arg != "allow_other"]
return subprocess.run(sshfs_args, capture_output=True, text=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] The retry filter [arg for arg in sshfs_args if arg != "allow_other"] removes only the string "allow_other" but leaves the preceding "-o" flag in the list. Since sshfs args are structured as pairs like ["-o", "option_value"], this produces something like [..., "-o", "-o", "next_opt"] or a trailing ["-o"], which is an invalid command.

Suggested fix: remove the -o/allow_other pair together. You could rebuild the args list filtering pairs, e.g., by iterating with an index and skipping both elements when the value matches.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch -- fixed. The _remove_allow_other method now properly removes both the -o flag and the allow_other value as a pair. The test test_mount_sshfs_allow_other_fallback verifies there are no orphaned -o flags after removal.

return result

def _create_temp_identity_file(self):
"""Create a temporary file with the SSH identity key, if configured"""
ssh_identity = self.identity
if not ssh_identity:
return None

temp_file = None
try:
temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_ssh_key')
temp_file.write(ssh_identity)
temp_file.close()
os.chmod(temp_file.name, 0o600)
self.logger.debug("Created temporary identity file: %s", temp_file.name)
return temp_file.name
except Exception as e:
self.logger.error("Failed to create temporary identity file: %s", e)
if temp_file:
try:
os.unlink(temp_file.name)
except Exception:
pass
raise
Comment on lines +270 to +289
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] In _create_temp_identity_file, if temp_file.write() raises an exception, temp_file.close() is never called, leaking the file descriptor.

Suggestion: close the file handle before unlinking in the error path, or use a try/finally around the write.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d9038e -- the error path now explicitly closes the file handle before unlinking, preventing the fd leak when write() raises.


def umount(self, mountpoint, *, lazy=False):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the absence of mount point, can you envision some way to remember the mountpoints and just unmount whatever we had mounted before? (i.e. some sort of temporary session files in .jumpstarter/shell/${lease-id} or similar (i.e. a hash of the socket path...)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Mountpoint session tracking (remembering mounts in .jumpstarter/shell/${lease-id} for argument-less unmount) adds non-trivial scope to this PR. Recommend tracking as a follow-up issue.

"""Unmount a previously mounted sshfs filesystem

Args:
mountpoint: Local mount point to unmount
lazy: If True, use lazy unmount
"""
mountpoint = os.path.realpath(mountpoint)

# Try fusermount first (Linux), fall back to umount (macOS)
fusermount = self._find_executable("fusermount3") or self._find_executable("fusermount")
if fusermount:
cmd = [fusermount, "-u"]
if lazy:
cmd.append("-z")
cmd.append(mountpoint)
else:
cmd = ["umount"]
if lazy:
cmd.append("-l")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] On macOS, fusermount is not available so _build_umount_cmd falls back to umount -l for lazy unmount. macOS umount does not support the -l flag and will error with "umount: illegal option -- l".

Suggestion: detect macOS (sys.platform == "darwin") and either map lazy to force (-f) or raise a clear error.

AI-generated, human reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d9038e -- _build_umount_cmd now checks sys.platform == "darwin" and uses -f (force) instead of -l (lazy) on macOS, since macOS umount does not support the -l flag. Added test_umount_lazy_macos_uses_force to verify.

cmd.append(mountpoint)

self.logger.debug("Running unmount command: %s", cmd)
result = subprocess.run(cmd, capture_output=True, text=True)

if result.returncode != 0:
stderr = result.stderr.strip()
raise click.ClickException(f"Unmount failed (exit code {result.returncode}): {stderr}")

click.echo(f"Unmounted {mountpoint}")

@staticmethod
def _find_executable(name):
"""Find an executable in PATH, return full path or None"""
import shutil
return shutil.which(name)
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from dataclasses import dataclass

from jumpstarter.common.exceptions import ConfigurationError
from jumpstarter.driver import Driver


@dataclass(kw_only=True)
class SSHMount(Driver):
"""SSHFS mount/umount driver for Jumpstarter

This driver provides remote filesystem mounting via sshfs.
It requires an 'ssh' child driver (SSHWrapper) which provides
SSH credentials and a 'tcp' sub-child for network connectivity.
"""

def __post_init__(self):
if hasattr(super(), "__post_init__"):
super().__post_init__()

if "ssh" not in self.children:
raise ConfigurationError(
"'ssh' child is required via ref to an SSHWrapper driver instance"
)

@classmethod
def client(cls) -> str:
return "jumpstarter_driver_ssh_mount.client.SSHMountClient"
Loading
Loading