Add client-side remote port forwarding#1066
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1066
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
There was a problem hiding this comment.
⚠️ Not ready to approve
The reverse-forward example sends cancel-tcpip-forward using the originally requested port (which can be 0) instead of the peer-selected bound port, risking leaked remote listeners and incorrect teardown behavior.
Pull request overview
Adds client-side remote port forwarding support (RFC 4254 tcpip-forward / cancel-tcpip-forward) to wolfSSH and updates the portfwd example + Expect-based tunnel test to exercise reverse forwarding.
Changes:
- Add public APIs
wolfSSH_FwdRemoteSetup()/wolfSSH_FwdRemoteCancel()to request/tear down remote listeners. - Implement dedicated global-request framing (
SendGlobalRequestFwd) to placewant-replybefore request-specific fields per RFC 4254. - Extend
examples/portfwdwith-rreverse mode and expandscripts/fwd.test.expectto test both local and reverse forwarding phases.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Declares new public APIs for client-side remote forwarding setup/cancel. |
| wolfssh/internal.h | Declares internal SendGlobalRequestFwd() helper behind WOLFSSH_FWD. |
| src/ssh.c | Implements wolfSSH_FwdRemoteSetup/Cancel() wrappers that send the new global requests. |
| src/internal.c | Adds SendGlobalRequestFwd() to build tcpip-forward / cancel-tcpip-forward packets with correct field ordering. |
| scripts/fwd.test.expect | Refactors the forwarding test into two phases (local + reverse) and improves process lifecycle handling. |
| examples/portfwd/portfwd.c | Adds -r reverse forwarding mode using forwarding + req-success/req-failure callbacks. |
| examples/echoserver/echoserver.c | Fixes forwarding listener FD handling by re-reading listenFd each loop iteration. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
⚠️ Not ready to approve
There are concrete correctness/robustness issues (public header exposes new APIs without matching feature-guarding, reverse setup loop mishandles valid wolfSSH_worker() statuses, and the expect harness can abort on transient ready-file read errors).
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1066
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1066
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
⚠️ Not ready to approve
The reverse-forwarding example has a few concrete robustness/behavior bugs (port parsing overflow-to-0, multiple inbound connection handling, and pending-state handling) that should be addressed before approval.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
6f77134 to
882ffb4
Compare
- Add wolfSSH_FwdRemoteSetup/Cancel to request tcpip-forward - Add SendGlobalRequestFwd to frame the request per RFC 4254 - Add portfwd -r reverse mode, driven by the fwd and req-success cbs - Wait for the peer's reply before -R reports ready, and take the bound port from it; with -f 0 that reply is the only source - Accept -f 0 in reverse mode, where it asks the peer to pick the port - Cancel names the port the peer bound, not the one requested, so a peer keyed on its listener's port can still find it - Add a req-failure cb so a refused forward is reported, not waited on Issue: ZD-21867
- ssh_worker() cached fwdCtx.listenFd at entry, while still -1 - The listener is only made later, from WOLFSSH_FWD_REMOTE_SETUP - FD_SET() and accept() then got a bogus fd, failing with EBADF, which broke the worker loop and dropped the session - Read listenFd from the context at each use, guarded on validity - agentListenFd has the same shape but is set up before ssh_worker() Issue: ZD-21867
- fwd.test only exercised portfwd's local forwarding mode - Factor the setup and line exchange into procs, run once per phase - Add a reverse phase, and a second one using -f 0, where the peer picks the port and only the tcpip-forward reply names it - Each phase uses its own forwarding ports, so it skips the previous phase's TIME_WAIT wait - Take the SSH port from echoserver -R, which is written once its listener is bound, so the client cannot race ahead of the listen() - Wait for a port in the ready files rather than their existence - Verified the reverse phase fails without the echoserver listener fix Issue: ZD-21867
Issue: ZD-28167