Add new ssh-mitm driver for jumpstarter#768
Conversation
WalkthroughAdds a new SSH MITM driver package with an SSHMITM driver implementation, example exporter YAML, tests, packaging (pyproject.toml), and README; the driver runs an ephemeral SSH server, relays client sessions to a TCP target via Paramiko, and exposes an async stream connect endpoint. Changes
Sequence DiagramsequenceDiagram
actor Client as SSH Client
participant MITM as SSHMITM (SSH Server)
participant DUT as Remote Target (DUT)
Client->>MITM: SSH Connect + Auth (client creds)
activate MITM
MITM->>MITM: MITMServerInterface negotiates session, records username
MITM->>DUT: Create SSH client -> Connect (host, port, identity)
activate DUT
DUT-->>MITM: SSH Ready / Auth OK
MITM-->>Client: Complete Auth / Session OK
alt Shell Session
Client->>MITM: Request Shell / PTY
MITM->>DUT: Open shell channel
MITM->>MITM: _proxy_channels() starts bidirectional threads
else Exec Command
Client->>MITM: Exec request
MITM->>DUT: Exec on DUT
MITM->>MITM: _proxy_channels() starts bidirectional threads
end
rect rgb(220,240,255)
Note over Client,DUT: Data proxied in separate daemon threads (stdin/stdout/stderr)
Client<<->>MITM: proxied streams
MITM<<->>DUT: proxied streams
end
Client->>MITM: Close Session
MITM->>DUT: Close channel & transport
deactivate DUT
MITM->>MITM: Cleanup transports, keys, threads
deactivate MITM
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested Reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (2)
184-199: Security: Host key checking is disabled.
StrictHostKeyChecking=noandUserKnownHostsFile=/dev/nullare appropriate here since the MITM proxy generates ephemeral host keys. Consider adding a comment explaining this is intentional for the MITM use case.ssh_command = [ ssh_binary, "-p", str(port), + # Disable host key checking - the MITM proxy generates ephemeral keys "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", f"{username}@{host}", ]
282-298: Port forwarding loop could use a more graceful shutdown mechanism.The
while True: time.sleep(1)pattern works but consumes unnecessary CPU cycles. Consider using an event-based wait.def _start_forward(self, local_host: str, local_port: int): """Expose the SSH MITM server on a local TCP port.""" click.echo("Starting local forward (Ctrl+C to stop)...") try: with TcpPortforwardAdapter( client=self, method="connect", local_host=local_host, local_port=local_port, ) as (bound_host, bound_port): click.echo(f"Local endpoint: {bound_host}:{bound_port}") click.echo(f"Example: ssh -p {bound_port} localhost") click.echo("Press Ctrl+C to stop forwarding.") - while True: - time.sleep(1) + import threading + stop_event = threading.Event() + stop_event.wait() except KeyboardInterrupt: click.echo("\nForward stopped.")This is a minor optimization and the current approach is functional.
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (2)
37-40: Unusual import placement after logging configuration.Imports after the
logging.getLogger()call is unconventional. Consider moving all imports to the top of the file.+from jumpstarter_driver_network.driver import TcpNetwork +from jumpstarter_driver_ssh.driver import SSHWrapper + import paramiko from anyio import get_cancelled_exc_class from anyio.from_thread import BlockingPortal from jumpstarter.common.exceptions import ConfigurationError from jumpstarter.driver import Driver, export, exportstream from jumpstarter.streams.common import create_memory_stream logging.getLogger("paramiko").setLevel(logging.WARNING) - -from jumpstarter_driver_network.driver import TcpNetwork -from jumpstarter_driver_ssh.driver import SSHWrapper
252-253: Consider making host key size configurable or using a larger default.While 2048-bit RSA is currently acceptable, NIST recommends transitioning to 3072-bit RSA for security through 2030. Since this is an ephemeral key, the performance impact of a larger key is minimal.
- self._host_key = paramiko.RSAKey.generate(2048) + self._host_key = paramiko.RSAKey.generate(3072)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
🧠 Learnings (13)
📓 Common learnings
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 608
File: packages/jumpstarter-cli/jumpstarter_cli/run.py:0-0
Timestamp: 2025-09-06T05:25:18.184Z
Learning: The Shell driver in jumpstarter can spawn multiple processes and leave them behind, creating zombie processes that need to be reaped even in non-PID 1 scenarios.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.pypackages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (3)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (8)
client(264-265)SSHMITM(202-524)SSHMITMError(43-44)close(113-137)close(520-524)_load_private_key(282-302)_handle_session(377-428)connect(432-482)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (4)
SSHMITMCommandRunResult(61-65)execute(300-313)run(99-121)run(315-317)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (5)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter/jumpstarter/streams/common.py (1)
create_memory_stream(40-45)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
SSHWrapper(9-50)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (1)
forward(162-170)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
[error] 15-15: RUFF: Import block is un-sorted or un-formatted.
🪛 GitHub Check: ruff
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
[failure] 173-173: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:173:1: W293 Blank line contains whitespace
[failure] 171-171: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:171:1: W293 Blank line contains whitespace
[failure] 165-165: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:165:1: W293 Blank line contains whitespace
[failure] 144-144: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:144:1: W293 Blank line contains whitespace
[failure] 134-134: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:134:1: W293 Blank line contains whitespace
[failure] 122-122: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:122:1: W293 Blank line contains whitespace
[failure] 80-80: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:80:1: W293 Blank line contains whitespace
[failure] 77-77: Ruff (C901)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:77:9: C901 cli is too complex (11 > 10)
[failure] 72-72: Ruff (W293)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:72:1: W293 Blank line contains whitespace
[failure] 15-31: Ruff (I001)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py:15:1: I001 Import block is un-sorted or un-formatted
🪛 Gitleaks (8.30.0)
packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml
[high] 13-16: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py
[high] 17-23: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (12)
packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml (1)
1-24: Example configuration looks good.This is a well-structured example showing both inline key and file-based key options. The placeholder private key is clearly marked with "(your private key here)" making it obvious this needs to be replaced. The static analysis warning about the private key is a false positive since this is intentionally example/placeholder content.
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (4)
16-24: Test key is appropriate for unit testing.The Ed25519 test key is a standard practice for unit testing SSH functionality. The Gitleaks warning is a false positive since this key is:
- Only used in test fixtures
- Not connected to any real system
- Clearly marked as test content
27-99: Comprehensive driver configuration tests.Good coverage of configuration validation including:
- Default configuration
- Missing TCP child error
- Missing identity error
- Both identities provided error
- gRPC access for default username
- Identity file loading
269-294: Security tests properly validate key protection.The tests correctly verify that
get_ssh_identitycannot be called via gRPC while remaining accessible internally. This is a critical security property for the MITM driver.
379-412: Stream connection test uses proper async patterns.The test correctly uses
@pytest.mark.anyioand verifies the session handler thread is started. The monkeypatching approach forparamiko.Transportis appropriate for testing stream behavior without actual SSH connections.packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (1)
254-267: Heredoc approach for command execution is sound but consider edge cases.The heredoc pattern with a unique token prevents shell interpolation issues. However, if the user's command itself contains the exact token string (extremely unlikely with UUID), it could cause issues. The current implementation is acceptable given the UUID-based token generation.
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (6)
48-137: StreamSocket implementation is well-designed.The bidirectional bridge using
socketpairand dedicated forwarding threads is a solid approach. The cleanup logic properly handles shutdown with timeouts and logs warnings for unclean shutdowns.
140-198: MITMServerInterface correctly delegates authentication to Jumpstarter.The design that accepts all SSH authentication methods is appropriate since real authentication happens through Jumpstarter's lease system. The optional username filtering adds an extra layer of access control.
314-315:AutoAddPolicyis appropriate for DUT connections.Since the driver connects to DUTs which may have varying or unknown host keys,
AutoAddPolicyis the right choice here. The security boundary is maintained by Jumpstarter's lease system and the private key staying on the exporter.
430-482: Stream bridging implementation is well-structured.The
connect()method correctly bridges Jumpstarter's async streams to Paramiko's synchronous socket interface. The exception handling during cleanup appropriately suppresses errors from already-closed connections. The documentation about gRPC ExecuteBatchError tracebacks is helpful.
484-518: Command execution implementation is robust.Good practices observed:
- Empty command check with informative error
shlex.joinfor safe command construction- Proper resource cleanup in
finallyblock- Exception handling returns error tuple instead of raising
520-524: Clean resource cleanup in close().The
close()method properly cleans up the SSH wrapper before calling the parent's close method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (2)
255-268: Potential command injection if user input contains the heredoc delimiter.While using a UUID-based heredoc token is a good approach, there's a theoretical (though extremely unlikely) edge case: if
cmdsomehow contains the exacttokenstring on its own line, it could prematurely terminate the heredoc. Given the UUID randomness this is practically impossible, but for defense in depth, consider validating or escaping.The current implementation is safe in practice due to the UUID's entropy. No change required unless you want extra paranoia.
296-299: Blocking sleep in forward loop could be improved with signal handling.The
time.sleep(1)loop works but is a polling pattern. This is acceptable for a simple forward command, but if responsiveness becomes an issue, consider usingsignal.pause()or an event-based approach.packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (4)
245-246: Consider using Ed25519 for the ephemeral host key.RSA 2048 is secure but Ed25519 is faster for key generation and handshakes. Since this is an ephemeral MITM server key (regenerated each time), the performance difference could be noticeable.
- # Generate ephemeral host key for MITM server - self._host_key = paramiko.RSAKey.generate(2048) + # Generate ephemeral host key for MITM server (Ed25519 for speed) + self._host_key = paramiko.Ed25519Key.generate()
343-348: Thread joins could block indefinitely if recv() hangs.If the SSH connection enters a pathological state where
recv()blocks forever without returning, thesejoin()calls will block indefinitely. Consider adding a timeout or ensuring channels are closed to unblock the recv calls.t1.start() t2.start() - t1.join() - t2.join() + t1.join(timeout=30) + t2.join(timeout=30) + if t1.is_alive() or t2.is_alive(): + self.logger.warning("Proxy threads did not exit cleanly")
397-410: Client channel may not be explicitly closed after shell sessions.In exec mode (lines 399-406),
client_channel.close()is called after forwarding exit status. In shell mode, the channel is only closed by_proxy_channelswhendst.close()is called in the forward function. This should work, but explicitly closingclient_channelin the finally block would be more robust.finally: + if client_channel: + try: + client_channel.close() + except Exception: + pass if dut_channel: try: dut_channel.close()
497-498: Decoding stdout/stderr with default UTF-8 may fail on binary output.If a command outputs binary data,
.decode()will raiseUnicodeDecodeError. Consider usingerrors='replace'orerrors='backslashreplace'for robustness.- stdout_data = stdout.read().decode() - stderr_data = stderr.read().decode() + stdout_data = stdout.read().decode(errors='replace') + stderr_data = stderr.read().decode(errors='replace')packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)
267-291: Consider adding a test for double-close safety.The cleanup tests verify that
close()doesn't raise, but consider adding a test that callsclose()twice to ensure idempotency.def test_close_idempotent(self): """Test that close() can be called multiple times safely""" instance = SSHMITM( children={"tcp": TcpNetwork(host="127.0.0.1", port=22)}, default_username="testuser", ssh_identity=TEST_SSH_KEY, ) instance.close() instance.close() # Should not raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/jumpstarter-driver-ssh-mitm/README.md(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-ssh-mitm/README.md
🧰 Additional context used
📓 Path-based instructions (6)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/pyproject.tomlpackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to **/*.py : Ruff should be used for code formatting and linting, excluding `jumpstarter-protocol` package
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.pypackages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.tomlpackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/pyproject.toml
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
SSHWrapper(9-50)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter/jumpstarter/streams/common.py (1)
create_memory_stream(40-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (14)
packages/jumpstarter-driver-ssh-mitm/pyproject.toml (3)
1-10: Project metadata and build configuration look good.The package correctly follows the naming pattern, includes Apache-2.0 licensing, specifies Python >=3.11, and uses standard hatch tooling with VCS versioning. The project metadata is complete with description, authors, readme, and homepage URLs.
Also applies to: 24-43
11-19: Verify all driver dependencies are necessary.The package depends on several driver packages (
jumpstarter-driver-composite,jumpstarter-driver-network,jumpstarter-driver-ssh). Confirm these are all actively used in the implementation by inspecting the imports indriver.pyandclient.py. If the SSH MITM driver can function independently for its core features, consider making unused dependencies optional or documenting their necessity.
21-22: No action needed on the entry point configuration.Jumpstarter does not require custom
entry-pointsfor driver discovery. Drivers are loaded by module/class path through YAML configuration, not via entry point registration. The current entry point at lines 21-22 does not affect driver discoverability.Likely an incorrect or invalid review comment.
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (3)
31-57: LGTM! Well-designed fallback command routing.The
DefaultCommandGroupimplementation elegantly handles the case where users type commands directly (e.g.,j ssh_mitm ls) by falling back to therunsubcommand, while still providing helpful typo detection viaget_close_matches.
190-194: Verify the security implications of disabled host key checking.
StrictHostKeyChecking=noandUserKnownHostsFile=/dev/nullare intentional for this MITM proxy setup (the MITM server has an ephemeral host key), but ensure this is documented as expected behavior so users understand they're connecting to a proxy, not directly to the DUT.The docstring and README should clarify that the client connects to the local MITM proxy (which has an ephemeral key), not directly to the DUT. This is a documentation concern, not a code issue.
301-318: LGTM! Clean delegation to driver via gRPC.The
executeandrunmethods correctly delegate to the driver'sexecute_commandgRPC method and wrap results in the appropriate dataclass. Therunalias is a sensible convenience.packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (4)
51-139: LGTM! Well-implemented bridge between async streams and paramiko.The
StreamSocketclass correctly uses a socketpair with forwarding threads to bridge Jumpstarter's async streams to paramiko's blocking socket API. The cleanup logic inclose()properly handles shutdown ordering and thread joining with timeouts.
141-194: LGTM! Auth delegation design is correct for MITM proxy.The
MITMServerInterfaceintentionally accepts all SSH authentication methods because the real authentication occurs through Jumpstarter's lease system. The optional username check adds a layer of validation without breaking the MITM flow.
463-469: Exception suppression during cleanup is intentional but worth noting.The broad exception catch (line 463) suppresses all errors during cleanup, which is documented in the docstring (lines 436-439). This prevents framework errors when clients disconnect abruptly. The approach is pragmatic but means some errors won't be logged.
512-516: LGTM! Proper cleanup delegation.The
close()method correctly cleans up the internal_ssh_wrapperbefore callingsuper().close()to handle child drivers.packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (4)
16-24: Test SSH key is a valid Ed25519 key format.Using a real key format (not just placeholder content) is good practice for testing key loading paths. This ensures the tests exercise actual parsing logic.
27-90: LGTM! Comprehensive configuration and access tests.The
TestSSHMITMDriverclass covers all configuration validation paths (missing tcp, missing identity, both identities) and correctly tests thatget_ssh_identityis not accessible via gRPC whileget_default_usernameis.
107-244: LGTM! Thorough client execution tests.The
TestSSHMITMClientclass covers important edge cases including command failures, multiple arguments, option-like arguments (e.g.,ps -ef), and empty commands. The mock setup properly isolates the tests from actual SSH connections.
353-384: LGTM! Async stream test validates session handler startup.The
test_connect_starts_sessiontest correctly verifies that theconnect()stream spins up the session handler thread. The use ofmonkeypatchto replaceparamiko.Transportwith a dummy is appropriate for unit testing without actual SSH connections.
|
@cursor can you review this? |
|
Please finish setting up background agents. Go to Cursor |
|
tests are failing, look at the logs, seems like some dependencies could be missing from the pyproject on the driver. |
|
@mangelajo yeah looking into it, will patch the fix soon, thanks :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (2)
297-323: Consider adding explicit SSH connection timeouts to avoid long hangs
_create_dut_clientcallsparamiko.SSHClient.connectwith all default timeouts. If the DUT host is firewalled or unresponsive, this can block for a long time, tying up whatever worker is runningexecute_commandor the stream entrypoint.It would be more robust to pass explicit timeouts (e.g., reusing
channel_timeoutor a dedicated configuration field) toconnect, so failures fail fast instead of hanging:- client.connect( - hostname=target_host, - port=target_port, - username=username, - pkey=pkey, - look_for_keys=False, - allow_agent=False, - ) + client.connect( + hostname=target_host, + port=target_port, + username=username, + pkey=pkey, + look_for_keys=False, + allow_agent=False, + timeout=self.channel_timeout, + )Adjusting or splitting the timeout if needed (e.g., separate connect vs. channel timeouts) would keep the driver responsive under network issues.
476-505: Make stdout/stderr decoding more robust to non‑UTF‑8 output
execute_commanddecodesstdoutandstderrwith the default.decode(), which assumes UTF‑8 and will raiseUnicodeDecodeErrorif the DUT emits arbitrary binary or legacy‑encoded output. That would drop you into theexceptblock and return(1, "", str(e)), losing any real exit code and partial output.You could avoid this by decoding with an explicit encoding and
errors="replace"(or"ignore") so that command failures vs. encoding issues stay distinguishable:- exit_code = stdout.channel.recv_exit_status() - stdout_data = stdout.read().decode() - stderr_data = stderr.read().decode() + exit_code = stdout.channel.recv_exit_status() + stdout_data = stdout.read().decode(errors="replace") + stderr_data = stderr.read().decode(errors="replace")This keeps
exit_code,stdout, andstderrmeaningful even when the output isn’t strict UTF‑8.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/jumpstarter-driver-ssh-mitm/README.md(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh-mitm/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-ssh-mitm/README.md
- packages/jumpstarter-driver-ssh-mitm/pyproject.toml
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to **/*.py : Ruff should be used for code formatting and linting, excluding `jumpstarter-protocol` package
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.pypackages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
SSHWrapper(9-50)packages/jumpstarter/jumpstarter/streams/common.py (1)
create_memory_stream(40-45)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (2)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (9)
client(257-258)SSHMITM(197-516)SSHMITMError(44-45)get_ssh_identity(266-268)close(116-138)close(512-516)_load_private_key(275-295)_handle_session(371-422)connect(426-474)packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (4)
SSHMITMCommandRunResult(61-66)execute(301-314)run(100-122)run(316-318)
🪛 Gitleaks (8.30.0)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py
[high] 17-23: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (3)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)
72-90: Good coverage of “key never leaves exporter” behaviorThe
test_identity_fileandTestSSHMITMSecuritycases nicely assert thatget_ssh_identityis not callable over gRPC (raisingDriverMethodNotImplemented) while still being accessible via the driver instance itself. This is exactly the boundary needed to enforce that private keys stay on the exporter only.No changes needed here.
Also applies to: 246-265
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/client.py (1)
31-173: CLI wiring forrun/shell/forwardlooks solid
DefaultCommandGroupplusignore_unknown_options/allow_extra_argsgives a nice UX:j ssh_mitm <cmd>falls back to therunsubcommand, while ssh‑style flags are passed through untouched. Therun,shell, andforwardcommands cleanly delegate intoSSHMITMClient.execute,_launch_native_ssh,_run_shell, and_start_forward, matching the usage documented in the module docstring and the tests.I don’t see functional issues here.
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (1)
232-269: Configuration validation and key handling match the intended security model
__post_init__correctly enforces that atcpchild exists, thatssh_identityandssh_identity_fileare mutually exclusive, and that at least one identity source is provided. Delegating username and key resolution to an internalSSHWrapperinstance, while only exportingget_default_usernameand keepingget_ssh_identityinternal, aligns well with the “private key stays on the exporter” guarantee.No issues here from a configuration or API-surface standpoint.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Sorry, I am requesting a medium/big change but I think it's necessary to avoid excessive duplication of code to maintain an debug in the long term.
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)
13-20: Private key in test file triggers secret scanner.The
TEST_SSH_KEYconstant contains an OpenSSH private key format that triggers Gitleaks. While this appears to be a test key, consider using a clearly non-secret dummy string since the tests mock Paramiko's key loading anyway.
🧹 Nitpick comments (2)
packages/jumpstarter-driver-ssh-mitm/README.md (1)
16-18: Add language specifier to fenced code block.The architecture diagram code block should have a language specifier for proper rendering. Use
textorplaintextfor ASCII diagrams.-``` +```text SSHWrapper --> SSHMITM --> TcpNetwork --> DUT</blockquote></details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (1)</summary><blockquote> `364-418`: **Consider extracting helper methods to reduce complexity.** The `_handle_session` method slightly exceeds the cyclomatic complexity threshold (11 vs 10). While the current structure is logical and readable, extracting the exit status handling (lines 395-402) and the cleanup block (lines 407-418) into separate methods could improve maintainability. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0940121e2ec8e7c321e7a489124fde287f3b7fe2 and a8d99c2d750ad6049f111e2aa39fc36adc3ad91b. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `uv.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (5)</summary> * `packages/jumpstarter-driver-ssh-mitm/README.md` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/pyproject.toml` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * packages/jumpstarter-driver-ssh-mitm/pyproject.toml </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py</summary> **📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)** > Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test` Files: - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>**/*.py</summary> **📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)** > Ruff should be used for code formatting and linting, excluding `jumpstarter-protocol` package Files: - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py</summary> **📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)** > Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server` Files: - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> </details><details> <summary>🧠 Learnings (7)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 608
File: packages/jumpstarter-cli/jumpstarter_cli/run.py:0-0
Timestamp: 2025-09-06T05:25:18.184Z
Learning: The Shell driver in jumpstarter can spawn multiple processes and leave them behind, creating zombie processes that need to be reaped even in non-PID 1 scenarios.Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.</details> <details> <summary>📚 Learning: 2025-11-27T09:58:41.875Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/*.py : Driver implementations should follow existing code style validated withmake lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` </details> <details> <summary>📚 Learning: 2025-11-27T09:58:55.346Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/ : Driver packages must implement adriver.pyfile containing the driver implementation**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>📚 Learning: 2025-11-27T09:58:55.346Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/ : Driver packages must implement aclient.pyfile containing the client implementation**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>📚 Learning: 2025-01-29T11:52:43.554Z</summary>Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>📚 Learning: 2025-11-27T09:58:41.875Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> <details> <summary>📚 Learning: 2025-11-27T09:58:41.875Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. UseCompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` </details> </details><details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (9)</summary> * `SSHMITM` (207-459) * `SSHMITMError` (32-33) * `client` (240-241) * `_get_ssh_identity` (243-252) * `close` (111-138) * `_load_private_key` (259-277) * `_handle_session` (364-418) * `connect` (422-459) * `MITMServerInterface` (141-203) </details> </blockquote></details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (2)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)</summary> * `TcpNetwork` (88-121) </details> <details> <summary>packages/jumpstarter/jumpstarter/streams/common.py (1)</summary> * `create_memory_stream` (40-45) </details> </blockquote></details> </details><details> <summary>🪛 GitHub Actions: Lint</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py</summary> [error] 281-281: Ruff check failed: W293 Blank line contains whitespace. Remove trailing spaces on blank line. Command: 'ruff check'. </details> </details> <details> <summary>🪛 GitHub Check: ruff</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py</summary> [failure] 362-362: Ruff (W293) packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py:362:1: W293 Blank line contains whitespace --- [failure] 110-110: Ruff (F401) packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py:110:45: F401 `jumpstarter.client.core.DriverMethodNotImplemented` imported but unused --- [failure] 109-111: Ruff (I001) packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py:109:1: I001 Import block is un-sorted or un-formatted </details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py</summary> [failure] 364-364: Ruff (C901) packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py:364:9: C901 `_handle_session` is too complex (11 > 10) --- [failure] 281-281: Ruff (W293) packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py:281:1: W293 Blank line contains whitespace </details> </details> <details> <summary>🪛 Gitleaks (8.30.0)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py</summary> [high] 13-19: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (private-key) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/README.md</summary> 16-16: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)</summary> * GitHub Check: Redirect rules - jumpstarter-docs * GitHub Check: Header rules - jumpstarter-docs * GitHub Check: Pages changed - jumpstarter-docs * GitHub Check: pytest-matrix (macos-15, 3.12) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.12) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.11) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.13) * GitHub Check: pytest-matrix (macos-15, 3.13) * GitHub Check: e2e </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-ssh-mitm/README.md (1)</summary><blockquote> `1-111`: **Well-structured documentation.** The README provides clear installation instructions, architecture overview, and configuration examples that align well with the driver implementation. </blockquote></details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)</summary><blockquote> `306-373`: **Good integration test for MITM proxy.** The `test_mitm_proxy_forwards_data` test comprehensively verifies the proxy behavior including data forwarding, exit status propagation, and cleanup. The mocking strategy properly isolates the test from actual SSH connections. </blockquote></details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml (1)</summary><blockquote> `1-33`: **Clear example configuration.** The exporter configuration example effectively demonstrates the driver hierarchy and both key configuration options. The comments provide helpful guidance for users. </blockquote></details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (5)</summary><blockquote> `39-139`: **Well-implemented stream-to-socket bridge.** The `StreamSocket` class properly handles bidirectional forwarding between async streams and blocking sockets. The cleanup logic with timeout handling and graceful thread shutdown is appropriate for this use case. --- `141-204`: **Authentication bypass is intentional and documented.** The `MITMServerInterface` correctly implements the MITM pattern where clients are pre-authenticated through Jumpstarter's lease system. The comment at lines 143-145 documents this design decision appropriately. --- `206-241`: **LGTM - Driver configuration and validation.** The `SSHMITM` class properly validates configuration constraints and generates an ephemeral host key to avoid fingerprint persistence issues. The decision to use `NetworkClient` as the client binding is correct for this network-layer driver. --- `420-459`: **Proper async stream endpoint implementation.** The `connect` method correctly implements the stream endpoint pattern with proper resource cleanup. The daemon thread with join timeout ensures the server thread doesn't leak. --- `293-294`: **Note: AutoAddPolicy disables host key verification for DUT.** `AutoAddPolicy` accepts any host key from the DUT without verification. This is typical for lab/test environments but worth noting. If stricter security is needed in the future, consider adding an optional `known_hosts` configuration. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (2)
13-20: Private key still embedded in test file.This was previously flagged and marked as addressed, but the
BEGIN OPENSSH PRIVATE KEYblock remains in the code and continues to trigger secret scanners.
361-363: Trailing whitespace on blank line.Line 362 contains trailing whitespace, which was flagged in a previous review.
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh-mitm/README.md (1)
16-18: Add language identifier to fenced code block.The architecture diagram block should specify a language (e.g.,
text) to satisfy markdown linting rules.Apply this diff:
-``` +```text SSHWrapper --> SSHMITM --> TcpNetwork --> DUTAs per coding guidelines, this aligns with markdown best practices. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a8d99c2d750ad6049f111e2aa39fc36adc3ad91b and 08876ac6bb3da10fdb4d9bf468165bac7f2a70af. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `uv.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (5)</summary> * `packages/jumpstarter-driver-ssh-mitm/README.md` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` (1 hunks) * `packages/jumpstarter-driver-ssh-mitm/pyproject.toml` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * packages/jumpstarter-driver-ssh-mitm/pyproject.toml * packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (2)</summary> <details> <summary>packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py</summary> **📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)** > Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test` Files: - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` </details> <details> <summary>**/*.py</summary> **📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)** > Ruff should be used for code formatting and linting, excluding `jumpstarter-protocol` package Files: - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` </details> </details><details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-11-27T09:58:41.875Z</summary>Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-/jumpstarter_driver_/*.py : Driver implementations should follow existing code style validated withmake lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test**Applied to files:** - `packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py (1)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver.py (9)</summary> * `SSHMITM` (207-459) * `SSHMITMError` (32-33) * `client` (240-241) * `_get_ssh_identity` (243-252) * `close` (111-138) * `_load_private_key` (259-277) * `_handle_session` (364-418) * `connect` (422-459) * `MITMServerInterface` (141-203) </details> </blockquote></details> </details><details> <summary>🪛 Gitleaks (8.30.0)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/jumpstarter_driver_ssh_mitm/driver_test.py</summary> [high] 13-19: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (private-key) </details> <details> <summary>packages/jumpstarter-driver-ssh-mitm/examples/exporter.yaml</summary> [high] 25-27: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (private-key) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>packages/jumpstarter-driver-ssh-mitm/README.md</summary> 16-16: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)</summary> * GitHub Check: Redirect rules - jumpstarter-docs * GitHub Check: Header rules - jumpstarter-docs * GitHub Check: Pages changed - jumpstarter-docs * GitHub Check: pytest-matrix (ubuntu-24.04, 3.13) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.11) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.12) * GitHub Check: e2e </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
I gave it a try, very cool!! ;D
jumpstarter ⚡local ➤ j ssh_mitm
[12/07/2025 20:56:43] INFO INFO:SSHMITM:MITM proxy established: client <-> DUT (mode=shell) driver.py:389
Last login: Sun Dec 7 20:56:04 2025 from ::1
➜ ~
➜ ~ exit
jumpstarter ⚡local ➤
2 participants
Adds a new SSH MITM driver that keeps SSH private keys securely on the exporter.
Implements a network layer driver (SSHMITM) that acts as a secure SSH proxy between
clients and DUTs, designed to be used as a child of SSHWrapper:
Architecture: SSHWrapper → SSHMITM → TcpNetwork → DUT
Usage: Configure as child of SSHWrapper, then use standard SSH commands:
j ssh_mitm <cmd -arg1 -arg2> # Execute command
j ssh_mitm # Interactive shell
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.