echoserver: answer session requests in callbacks - #1234
Conversation
The one server-side site that opens auth-agent@openssh.com sits inside wolfSSH_accept(), so an application driving its own channels cannot reach it: the session records the request and no channel follows. - add wolfSSH_AGENT_ChannelOpen(), the same open lifted out of accept(), which still calls it - it reports WS_BAD_ARGUMENT until the peer asks and is idempotent afterward, so an application can poll it - publish the agent on a queued open too, so a retry after WS_WANT_WRITE finds it rather than opening a second channel and leaking the first
A server that wants to own its channels had no way to get them: accept() ran the session state machine to the end, and a shell, exec or subsystem request with no callback registered was granted regardless. - add wolfSSH_CTX_SetAppChannels() and wolfSSH_SetAppChannels(), off by default, a byte on the context copied into the session - on, accept() returns once the user is authenticated, and a session request with no callback behind it is refused: nothing is left to serve - keep the stop state out of the pending-send advance, so a re-entry with queued output cannot step over where this call is meant to stop - stop early only while the session is short of that state, so turning the mode on afterward cannot leave the loop hunting a state it went past - teach wolfSSH_SFTP_accept() that the mode parks accept() short of an established session, so it stops redoing the handshake on every poll
wolfSSH_SetAppChannels() changes where wolfSSH_accept() stops and what becomes of a session request with no callback behind it, so both modes are exercised. - regress.c drives a server with the pivot on, one with a shell callback and one without, and checks accept() stops at ACCEPT_SERVER_USERAUTH_SENT - regress.c pins the context setter, the session's inheritance of it, and that turning it on after accept() established the session still returns - unit.c checks DoChannelRequest() refuses a shell, exec and subsystem request with no callback once the pivot is on - the untouched AssertHandshakeSucceeds() is the regression gate for a server that registers nothing
An application that binds an "scp ..." command to a channel itself has no way to run the transfer; wolfSSH_accept() did it through a WS_SCP_INIT re-entry only that state machine can drive. - add wolfSSH_SCP_accept(), a wrapper over DoScpRequest() reporting WS_SCP_COMPLETE for any non-negative result, as accept() does - a receive-side want reaches the wrapper as a generic error with the want in ssh->error, so report the want itself and let the caller retry - state that resume contract beside the prototype, and clear a stale want on entry the way the other re-entrant entry points do
With -A the echoserver drives its own channels: accept() stops at userauth and the callbacks below start the shell, SFTP or SCP session. Off by default, so the path this example has always taken stays the one an unflagged run demonstrates. The two are exclusive, since the callbacks answer the requests the accept state machine otherwise answers itself. - wsShellStartCb() forks the pty, so it is registered in both modes and claims the channel only once there is a shell behind it - wsExecStartCb() takes an "scp " command as a transfer and any other command as an echo session; wsSubsysStartCb() guards a NULL command, which a truncated request leaves behind - ssh_worker() drives the session through shellCtx.appFd, and claims the channel itself when no callback did - resume a subsystem accept that returns a want, waiting on the socket between attempts rather than spinning - close the accepted socket again, and clear fwdFd on EOF or reset
There was a problem hiding this comment.
🟡 Changes recommended
Public-header documentation for new APIs/modes is currently inconsistent with the implemented return semantics and supported flows, which can mislead API consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends wolfSSH’s server-side channel handling to support an opt-in, application-driven channel mode where wolfSSH_accept() stops after user authentication and the application drives session/channel progress via callbacks and wolfSSH_worker(). It also updates the echoserver example and adds/extends tests to cover the new behavior and retry semantics for non-blocking flows.
Changes:
- Add application-driven channel mode (
wolfSSH_CTX_SetAppChannels(),wolfSSH_SetAppChannels()) and adjustwolfSSH_accept()/channel-request handling accordingly. - Expose server-side helpers for app-driven flows (notably
wolfSSH_SCP_accept()andwolfSSH_AGENT_ChannelOpen()), and adapt SFTP accept behavior in this mode. - Update echoserver to support a
-Amode (callbacks start shell/SFTP/SCP sessions) and add regression/unit test coverage.
File summaries
| File | Description |
|---|---|
| wolfssh/wolfscp.h | Documents new server-side SCP accept API for application-driven transfers. |
| wolfssh/ssh.h | Documents new application-driven channel mode and adds public setters. |
| wolfssh/internal.h | Adds appChannels flag to WOLFSSH_CTX and WOLFSSH structs. |
| wolfssh/agent.h | Declares wolfSSH_AGENT_ChannelOpen() for app-driven agent forwarding channel open. |
| src/ssh.c | Updates wolfSSH_accept() stop-state logic; adds setters for app-channels; refactors agent open to new helper. |
| src/internal.c | Changes default “no callback” behavior for shell/exec/subsystem requests when app-driven mode is enabled. |
| src/wolfsftp.c | Makes wolfSSH_SFTP_accept() treat app-driven accept stop-state as “accept done” for callers. |
| src/wolfscp.c | Implements wolfSSH_SCP_accept() wrapper over SCP request handling with WANT propagation. |
| src/agent.c | Implements wolfSSH_AGENT_ChannelOpen() helper for server-side agent channel open. |
| tests/unit.c | Adds unit coverage ensuring no-callback session requests are refused in app-driven mode. |
| tests/regress.c | Adds regression coverage for accept stopping at userauth, callback dispatch, inheritance, and late-enable behavior. |
| tests/api.c | Adjusts keyboard-interactive test client args to avoid shell-callback refusal in echo mode. |
| examples/echoserver/echoserver.c | Adds -A mode and callback-driven session start; updates non-blocking retry loops for SFTP/SCP; refactors shell startup into callback. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* Server side. Opens the auth-agent@openssh.com channel back to the client | ||
| * once an auth-agent-req@openssh.com request has set the session up for it. | ||
| * wolfSSH_accept() does this itself on the default path; an application that | ||
| * drives its own channels returns from accept() before that point and calls | ||
| * this instead. Idempotent, so it is safe to poll while waiting for the | ||
| * peer's request. Returns WS_SUCCESS, or WS_BAD_ARGUMENT when the session | ||
| * never asked for agent forwarding. */ |
| * The mode drives the session channels itself, so it does not combine with | ||
| * the built-in wolfSSH_SFTP_accept() and WS_SCP_INIT entry points; an | ||
| * application using those leaves this off. */ |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1234
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| #endif | ||
| #ifdef WOLFSSH_AGENT | ||
| WS_SOCKET_T agentFd = -1; | ||
| WS_SOCKET_T agentListenFd = threadCtx->agentCtx.listenFd; |
There was a problem hiding this comment.
Stale agentListenFd cached before the agent listener is created in application-driven mode · Logic errors
agentListenFd is cached once before the loop. With -A, the new wolfSSH_AGENT_ChannelOpen() poll fires WOLFSSH_AGENT_LOCAL_SETUP mid-loop, which sets agentCtx.listenFd and flips agentCtx.state to APP_STATE_LISTEN while the cached copy is still -1, so FD_SET(-1, &readFds) and accept(-1, ...) run.
Fix: Read threadCtx->agentCtx.listenFd each pass and gate the FD_SET/accept on it being >= 0, as the WOLFSSH_FWD listener block does.
| return 1; | ||
| } | ||
|
|
||
| childPid = forkpty(&threadCtx->shellCtx.appFd, NULL, NULL, NULL); |
There was a problem hiding this comment.
Repeated shell/exec requests re-fork the pty and leak the previous master fd and child · Resource leaks
wsShellStartCb() has no guard against being invoked more than once, and DoChannelRequest() dispatches every shell (and, via wsExecStartCb(), every non-scp exec) request. An authenticated client repeating the request in the ssh_worker() loop forks a new /bin/sh each time and overwrites threadCtx->shellCtx.appFd, leaking every prior pty master fd until the server exhausts descriptors.
Fix: Reject the request in wsShellStartCb() when threadCtx->shellCtx.appFd >= 0 or the state is already APP_STATE_CONNECTED.
| cnt_r = wolfSSH_worker(ssh, &lastChannel); | ||
| #ifdef WOLFSSH_SFTP | ||
| if (threadCtx->doSftp) { | ||
| return WS_SFTP_COMPLETE; |
There was a problem hiding this comment.
SFTP/SCP early returns in ssh_worker() bypass the pty close · Resource leaks
The new return WS_SFTP_COMPLETE / return WS_SCP_INIT skip the WCLOSESOCKET(threadCtx->shellCtx.appFd) cleanup that follows the loop. A client that requests shell and then subsystem sftp or exec scp ... leaves the pty master fd open and the /bin/sh child running for the remaining lifetime of the process, which accumulates across connections under multipleConnections.
Fix: Close and reset threadCtx->shellCtx.appFd before returning WS_SFTP_COMPLETE or WS_SCP_INIT.
| } | ||
|
|
||
| if (ssh != NULL) | ||
| ssh->error = ret; |
There was a problem hiding this comment.
wolfSSH_AGENT_ChannelOpen() stores WS_BAD_ARGUMENT into ssh-error on its documented polling path · Channel handling errors
The function is documented as pollable and returns WS_BAD_ARGUMENT while ssh->useAgent is 0, but it also writes that value into ssh->error. wolfSSH_accept() (src/ssh.c:598) returns WS_INVALID_STATE_E for any non-zero ssh->error, so polling for the agent channel poisons the session's error slot and breaks a concurrently driven accept.
Fix: Only assign ssh->error for real failures, leaving it untouched on the not-yet-requested and idempotent-success paths.
Stacked on #1233, and it also carries #1231's commit verbatim, so only
the last commit is new here. With -A the echoserver drives its own
channels: accept() stops at userauth and the callbacks below start the
shell, SFTP or SCP session. Off by default, and the two are exclusive.
claims the channel only once there is a shell behind it.
command as an echo session; wsSubsysStartCb() guards a NULL command.
channel itself when no callback did.
between attempts rather than spinning.