feat(tunnels): import SSH forwards as tunnels + working per-tunnel au…#81
feat(tunnels): import SSH forwards as tunnels + working per-tunnel au…#81BungeeDEV wants to merge 1 commit into
Conversation
…to-start Parse LocalForward/RemoteForward/DynamicForward from ~/.ssh/config and import each as a tunnel bound to the host. Make auto-start actually fire: tunnels flagged auto_start start when their host connects and stop on the host's last disconnect, with a global "auto-start by default" setting that seeds new (and imported) tunnels. - parser: Local/Remote/Dynamic directives ([bind:]port, [ipv6]:port, host:hostport); idempotent re-import via a natural key + host reuse - backend forwarding: Local + Dynamic (SOCKS5) + Remote (tcpip_forward); each tunnel owns its own SSH connection with clean teardown and is ProxyJump-aware for Local/Dynamic - lifecycle: start on connect_saved_host, stop when the host's last session disconnects; per-instance teardown guard so a reconnect can't let a stale listener clobber the restarted tunnel - surface bind/start failures as toasts (friendly hint for ports <1024) - frontend: forward-type selector, type/SOCKS badges, global Settings toggle seeding new tunnels' auto_start Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8e8ee5a to
1bbb492
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tokio::spawn(async move { | ||
| cancel_token.cancelled().await; | ||
| let _ = handle | ||
| .cancel_tcpip_forward(bind_for_cancel, listen_port) | ||
| .await; | ||
| let _ = handle | ||
| .disconnect(russh::Disconnect::ByApplication, "", "en") | ||
| .await; | ||
| drop(handle); | ||
|
|
||
| // Only announce Stopped if this instance is still the live one (see | ||
| // the listener-tunnel teardown for the reasoning). | ||
| if tunnels | ||
| .remove_if(&rid, |_, t| t.instance_id == instance_id) | ||
| .is_some() | ||
| { | ||
| let _ = app_handle.emit( | ||
| "pf:status", | ||
| &TunnelStatus { | ||
| rule_id: rid, | ||
| status: TunnelState::Stopped, | ||
| local_port: listen_port, | ||
| connections: 0, | ||
| error: None, | ||
| }, | ||
| ); | ||
| } | ||
| let _ = app_handle.emit( | ||
| "pf:status", | ||
| &TunnelStatus { | ||
| rule_id: rid, | ||
| status: TunnelState::Stopped, | ||
| local_port: actual_port, | ||
| connections: 0, | ||
| error: None, | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The spawned task blocks indefinitely on cancel_token.cancelled().await. There is no mechanism to detect a broken SSH connection — if the server crashes or the network goes down, russh calls the handler's disconnected() (not implemented by RemoteForwardHandler, so the default no-op fires), but the cancel token is never signalled. The entry in self.tunnels is never removed and no pf:status Stopped event is emitted. The UI shows the tunnel as live even though the server-side listener is gone and all new remote connections silently fail.
RemoteForwardHandler should implement disconnected() and cancel the token (or use a separate JoinHandle for the russh connection task and select! on it alongside the cancel token).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/portforward/manager.rs
Line: 377-404
Comment:
**Remote tunnel stays "Active" after SSH connection drops**
The spawned task blocks indefinitely on `cancel_token.cancelled().await`. There is no mechanism to detect a broken SSH connection — if the server crashes or the network goes down, `russh` calls the handler's `disconnected()` (not implemented by `RemoteForwardHandler`, so the default no-op fires), but the cancel token is never signalled. The entry in `self.tunnels` is never removed and no `pf:status Stopped` event is emitted. The UI shows the tunnel as live even though the server-side listener is gone and all new remote connections silently fail.
`RemoteForwardHandler` should implement `disconnected()` and cancel the token (or use a separate `JoinHandle` for the russh connection task and `select!` on it alongside the cancel token).
How can I resolve this? If you propose a fix, please make it concise.
| Ok(true) | ||
| } | ||
|
|
||
| Ok(()) | ||
| async fn server_channel_open_forwarded_tcpip( | ||
| &mut self, | ||
| channel: russh::Channel<client::Msg>, | ||
| _connected_address: &str, | ||
| _connected_port: u32, | ||
| _originator_address: &str, | ||
| _originator_port: u32, |
There was a problem hiding this comment.
Each server_channel_open_forwarded_tcpip call pumps with CancellationToken::new() — a fresh, never-cancelled token — rather than reusing the tunnel's own cancel token. When stop_tunnel fires the tunnel's token, already-established remote channels keep running to natural termination. By contrast, the local-forward path propagates the tunnel's cancel token to every per-connection proxy_to_remote call. Using the tunnel-level cancel token here would make behaviour consistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/portforward/manager.rs
Line: 632-641
Comment:
**In-progress remote-forward connections are not cancelled when tunnel is stopped**
Each `server_channel_open_forwarded_tcpip` call pumps with `CancellationToken::new()` — a fresh, never-cancelled token — rather than reusing the tunnel's own cancel token. When `stop_tunnel` fires the tunnel's token, already-established remote channels keep running to natural termination. By contrast, the local-forward path propagates the tunnel's cancel token to every per-connection `proxy_to_remote` call. Using the tunnel-level cancel token here would make behaviour consistent.
How can I resolve this? If you propose a fix, please make it concise.| // Ask the server to start listening. A bind conflict on the *server* is | ||
| // reported here as a request denial. | ||
| handle | ||
| .tcpip_forward(bind_address.clone(), listen_port) | ||
| .await | ||
| .map_err(|e| { | ||
| SshError::IoError(format!( | ||
| "server refused to forward {bind_address}:{listen_port}: {e}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
tcpip_forward returns Ok(u32) where the value is the server-allocated port when the requested port is 0. That value is silently discarded here. If a port-0 remote tunnel is ever created (the UI currently blocks it, but it can arrive via the import path), local_port in the ActiveTunnel entry would stay 0 and the status event would also report 0, giving users no indication of which port to connect to on the remote side.
| // Ask the server to start listening. A bind conflict on the *server* is | |
| // reported here as a request denial. | |
| handle | |
| .tcpip_forward(bind_address.clone(), listen_port) | |
| .await | |
| .map_err(|e| { | |
| SshError::IoError(format!( | |
| "server refused to forward {bind_address}:{listen_port}: {e}" | |
| )) | |
| })?; | |
| // Ask the server to start listening. A bind conflict on the *server* is | |
| // reported here as a request denial. | |
| let actual_listen_port = handle | |
| .tcpip_forward(bind_address.clone(), listen_port) | |
| .await | |
| .map_err(|e| { | |
| SshError::IoError(format!( | |
| "server refused to forward {bind_address}:{listen_port}: {e}" | |
| )) | |
| })?; | |
| // When listen_port == 0 the server chooses; use the returned port. | |
| let listen_port = if listen_port == 0 { actual_listen_port } else { listen_port }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/portforward/manager.rs
Line: 330-339
Comment:
`tcpip_forward` returns `Ok(u32)` where the value is the server-allocated port when the requested port is 0. That value is silently discarded here. If a port-0 remote tunnel is ever created (the UI currently blocks it, but it can arrive via the import path), `local_port` in the `ActiveTunnel` entry would stay 0 and the status event would also report 0, giving users no indication of which port to connect to on the remote side.
```suggestion
// Ask the server to start listening. A bind conflict on the *server* is
// reported here as a request denial.
let actual_listen_port = handle
.tcpip_forward(bind_address.clone(), listen_port)
.await
.map_err(|e| {
SshError::IoError(format!(
"server refused to forward {bind_address}:{listen_port}: {e}"
))
})?;
// When listen_port == 0 the server chooses; use the returned port.
let listen_port = if listen_port == 0 { actual_listen_port } else { listen_port };
```
How can I resolve this? If you propose a fix, please make it concise.| // The backend loads the rule (forward type + params) by id; hostId is | ||
| // kept in the signature for callers but no longer sent. | ||
| void hostId; | ||
| const status = await invoke<TunnelStatus>("pf_start_tunnel", { ruleId }); |
There was a problem hiding this comment.
void hostId idiom suppresses the "unused variable" lint but leaves dead code in place. Since the backend now resolves everything from ruleId, hostId can be dropped from the call.
| // The backend loads the rule (forward type + params) by id; hostId is | |
| // kept in the signature for callers but no longer sent. | |
| void hostId; | |
| const status = await invoke<TunnelStatus>("pf_start_tunnel", { ruleId }); | |
| const status = await invoke<TunnelStatus>("pf_start_tunnel", { ruleId }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/stores/port-forward-store.ts
Line: 102-105
Comment:
The `void hostId` idiom suppresses the "unused variable" lint but leaves dead code in place. Since the backend now resolves everything from `ruleId`, `hostId` can be dropped from the call.
```suggestion
const status = await invoke<TunnelStatus>("pf_start_tunnel", { ruleId });
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| #[allow(clippy::too_many_arguments)] | ||
| pub fn find_pf_rule_id_by_natural_key( |
There was a problem hiding this comment.
find_pf_rule_id_by_natural_key has 6 non-self parameters, which is below Clippy's default limit of 7, so the #[allow] is unnecessary.
| #[allow(clippy::too_many_arguments)] | |
| pub fn find_pf_rule_id_by_natural_key( | |
| pub fn find_pf_rule_id_by_natural_key( |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/db/mod.rs
Line: 1573-1574
Comment:
`find_pf_rule_id_by_natural_key` has 6 non-`self` parameters, which is below Clippy's default limit of 7, so the `#[allow]` is unnecessary.
```suggestion
pub fn find_pf_rule_id_by_natural_key(
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
This pull request improves the SSH config import process by making port-forwarding (tunnel) imports idempotent and more robust. It introduces a natural-key lookup for port-forwarding rules to avoid duplicate tunnels when re-importing the same SSH config, and adds support for parsing and importing SSH port-forwarding directives directly from config files. The changes are covered by new tests to ensure correctness.
SSH config import improvements:
Added parsing and extraction of port-forwarding directives (
LocalForward,RemoteForward,DynamicForward) from SSH config files, mapping them to a newSshForwardstruct and including them inSshConfigEntryandSshConfigImportEntry(src-tauri/src/import/mod.rs). [1] [2] [3] [4] [5] [6]On import, port-forwarding rules are now created per host entry, but only if an identical rule does not already exist (idempotency). This is achieved by checking for an existing rule using a new natural-key lookup before creating a new one (
src-tauri/src/import/commands.rs). [1] [2] [3]Database enhancements:
find_pf_rule_id_by_natural_keyandget_pf_rulemethods toHostDbto support natural-key lookup and retrieval of port-forwarding rules, enabling idempotent import and easier rule management (src-tauri/src/db/mod.rs). [1] [2]Testing and reliability:
src-tauri/src/db/mod.rs,src-tauri/src/import/commands.rs). [1] [2]Greptile Summary
This PR adds SSH config forward-directive import (LocalForward/RemoteForward/DynamicForward → tunnel records), makes both host and tunnel import idempotent via natural-key lookups, introduces Dynamic (SOCKS5) forwarding, adds per-tunnel SSH connections for all forward types, and wires up auto-start/auto-stop of tunnels on host connect/disconnect.
(host, user, port)before inserting, and tunnel records are matched by their natural key before creating — re-importing the same SSH config is now a no-op, covered by new tests.Dynamic(SOCKS5 proxy with a hand-rolled negotiation loop) andRemote(ssh -R, server-side listener) are fully implemented end-to-end from the DB through the manager to the UI.session_hostsinSshManagertracks which sessions belong to which saved host so the last-disconnect event correctly tears down auto-started tunnels; reconnect inDisconnectOverlayintentionally delays the old-session teardown to avoid a false "host disconnected" flash.Confidence Score: 3/5
Safe to merge for local and dynamic tunnel types; remote forwarding tunnels will silently appear as Active after the underlying SSH connection drops and never recover on their own.
The import and local/dynamic forwarding paths are well-structured and tested. The remote-tunnel lifecycle has a concrete gap: the spawned task parks on the cancel token with no SSH-connection-loss detection, so a dropped server connection leaves the tunnel stuck forever as Active in the map with no Stopped event to the UI. For a feature that is on by default, this is likely to surface quickly in real use.
Files Needing Attention: src-tauri/src/portforward/manager.rs — specifically start_remote_tunnel and RemoteForwardHandler; the per-connection cancel-token inconsistency is a secondary concern in the same file.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Frontend UI participant Cmd as portforward/commands participant Mgr as PortForwardManager participant SSH as SshManager participant DB as HostDb Note over UI,DB: Host connect + auto-start tunnels UI->>SSH: ssh_connect(host_id) SSH-->>UI: SessionId SSH->>Cmd: start_auto_tunnels_for_host(host_id) Cmd->>DB: list_pf_rules(host_id) DB-->>Cmd: "rules where auto_start=true" loop each auto_start rule Cmd->>DB: build_host_config_blocking Cmd->>Mgr: "start_tunnel(rule, auto_started=true)" Mgr-->>UI: pf:status Active end Note over UI,DB: Manual tunnel start UI->>Cmd: pf_start_tunnel(rule_id) Cmd->>DB: get_pf_rule(rule_id) Cmd->>Mgr: "start_tunnel(rule, auto_started=false)" Mgr-->>UI: pf:status Active Note over UI,DB: Host disconnect UI->>SSH: ssh_disconnect(session_id) SSH->>SSH: remove from session_hosts alt last session for host SSH->>Cmd: stop_auto_tunnels_for_host Mgr-->>UI: pf:status Stopped per tunnel end Note over UI,DB: SSH config import UI->>Cmd: import_save_ssh_hosts(entries) Cmd->>DB: list_hosts() snapshot loop each entry Cmd->>DB: find existing or save_host loop each forward Cmd->>DB: find_pf_rule_id_by_natural_key alt not found Cmd->>DB: create_pf_rule end end end Cmd-->>UI: ImportResultPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(tunnels): import SSH forwards as tu..." | Re-trigger Greptile