ssh: add opt-in application-driven channels - #1233
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are API contract/documentation mismatches with observable runtime behavior (late enable semantics and return-code documentation) that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an opt-in “application-driven channels” mode for the server side, allowing wolfSSH_accept() to return immediately after user authentication so the application can drive channel lifecycle and request handling via wolfSSH_worker(). It also refactors agent forwarding channel opening for reuse outside wolfSSH_accept(), and updates SFTP acceptance logic and tests to cover both default and application-driven modes.
Changes:
- Add
wolfSSH_CTX_SetAppChannels()/wolfSSH_SetAppChannels()to optionally stopwolfSSH_accept()at post-auth and reject shell/exec/subsystem requests that have no registered callback in this mode. - Update
wolfSSH_accept()state advancement/stop behavior and extract agent channel open intowolfSSH_AGENT_ChannelOpen(). - Extend unit/regress tests to exercise both modes; adjust
wolfSSH_SFTP_accept()to treat the post-auth stop state as “done” for its accept precondition.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Adds public API and documentation for application-driven channel mode. |
| wolfssh/internal.h | Adds appChannels flag to context/session internal structs. |
| wolfssh/agent.h | Declares wolfSSH_AGENT_ChannelOpen() and documents intended usage. |
| src/ssh.c | Implements new setters and modifies wolfSSH_accept() stop/advance behavior; switches to wolfSSH_AGENT_ChannelOpen(). |
| src/internal.c | Inherits appChannels from context and changes channel-request default handling under app-driven mode. |
| src/agent.c | Implements wolfSSH_AGENT_ChannelOpen() extracted from accept flow. |
| src/wolfsftp.c | Treats post-auth stop state as “accept done” for SFTP accept gating. |
| tests/unit.c | Adds unit coverage ensuring no-callback shell/exec/subsystem requests are refused under app-driven mode. |
| tests/regress.c | Adds regression harness/tests for accept stopping point, inheritance, and late-enable behavior. |
Review details
- Files reviewed: 9/9 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.
8121dac to
127b742
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1233
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. Reported findings require changes before merge.
| } | ||
|
|
||
| if (ssh != NULL) | ||
| ssh->error = ret; |
There was a problem hiding this comment.
wolfSSH_AGENT_ChannelOpen() latches its benign "not requested yet" return into ssh-error · Incorrect error handling
Every return is written into ssh->error, including the WS_BAD_ARGUMENT reported while the peer has not asked for agent forwarding — the polling agent.h documents as safe. wolfSSH_accept() (src/ssh.c:598) clears only the want/auth-pending codes, so a poll poisons the session and later accepts return WS_INVALID_STATE_E. The success path likewise clears a latched WS_WANT_WRITE. Sibling wolfSSH_AGENT_Relay() sets ssh->error only on genuine failure.
Fix: Record ssh->error only for genuine failures, leaving it untouched on WS_SUCCESS and on the WS_BAD_ARGUMENT no-agent-requested return.
| * application-driven mode accept() parks at ACCEPT_SERVER_USERAUTH_SENT | ||
| * and never advances, so that state counts as done here. */ | ||
| if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED | ||
| && !(ssh->appChannels |
There was a problem hiding this comment.
wolfSSH_SFTP_accept() serves SFTP without a granted subsystem request in app-channels mode · Channel handling errors
The new guard drops into the SFTP state machine as soon as ssh->appChannels is set and userauth completed, with no requirement that the peer opened a channel or that a subsystem sftp request was granted. With appChannels on and no channelReqSubsysCb, DoChannelRequest replies CHANNEL_FAILURE (rej = ssh->appChannels) yet SFTP is still served on that channel. Adjacent to known finding #8852, which covers state committed in DoChannelRequest after a callback rejects; this is a separate gate removed in wolfSSH_SFTP_accept.
Fix: Gate the app-channels bypass on the session request having been granted (e.g. clientState >= CLIENT_DONE and session type SUBSYSTEM/sftp) rather than on userauth alone.
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 on a client session, and is idempotent after, 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 - flush what is left of a queued open on the next call, rather than reporting a success the peer never saw - record ssh->error from the send alone, so neither a poll ahead of the request nor a failed allocation stops accept() continuing
wolfSSH_AGENT_ChannelOpen() answers a poll on a session that is over with WS_FATAL_ERROR and WS_DISCONNECT in ssh->error, the shape every other public sender uses: no channel opened, nothing on the wire, RFC 4253 section 11.1. wolfSSH_accept() gates the open it drives, so the new public entry point is the only way in. - promote SendAfterDisconnect() to WOLFSSH_LOCAL so agent.c uses the same helper as every other public sender - leave an open queued before the disconnect unflushed, the rule wolfSSH_shutdown() applies to all but its own disconnect - keep WS_DISCONNECT in ssh->error at the accept() call site, which used to overwrite it with the status the open returns
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
DoChannelRequest() reads ssh->appChannels when the request arrives, so turning the mode on after accept() established the session still refuses an uncallbacked shell, exec or subsystem request from then on. Only accept()'s stopping point is pinned, by the guard around stopState. - say the flag reaches the requests that follow, and that what it cannot do is move where accept() returns - drive a shell request over the wire in both modes from the late-enable test, pinning the behaviour the header now describes
127b742 to
f359ec4
Compare
Stacked on #1230, whose two commits are the first of the five here; review
the last three. 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.
default. On, accept() returns once the user is authenticated and a
session request with no callback behind it is refused.
only while the session is short of that state.
established session, so it stops redoing the handshake on every poll.
an uncallbacked request once the pivot is on.