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.
127b742 to
f359ec4
Compare
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
f359ec4 to
ab06a5f
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.
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: 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.
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 - regress.c re-enters a parked accept() with output still queued, which is the one path that flushes before reading the state, and pins that it leaves the state on the stop - unit.c checks DoChannelRequest() refuses a shell, exec and subsystem request with no callback once the pivot is on
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
In application-driven mode wolfSSH_accept() parks at userauth, so the sftp test its divert applies never runs. wolfSSH_SFTP_accept() applies it itself: the session channel must be a subsystem the application's callback granted sftp on, or the call returns WS_INVALID_STATE_E and leaves the wire alone without recording an error. - gate the app-channels branch on wolfSSH_GetSessionType() and wolfSSH_GetSessionCommand(), the same test accept() makes - ask for that grant in every accept state: below the user-auth stop accept() returns with no channel open, and past the stop there is no accept() left that could have checked anything - say in ssh.h that the mode serves SFTP through that grant and never reaches the SCP entry point - regress.c refuses the call with no channel, ahead of accept(), on a granted shell and on an established one, and serves an INIT on a granted sftp subsystem
33d707a to
9f2b902
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.
9f2b902 to
f8d3591
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: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
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: 1
Required changes (1)
Exec and subsystem callbacks leave a dangling channel access
File: src/internal.c:13153
Function: DoChannelRequest
Category: API contract violations
DoChannelRequest() permits callbacks to free their channel, yet exec and subsystem paths dereference channel->command afterward when logging is enabled. Distinct from known #11674: parsing succeeds here; callback lifetime is the fault.
Related known finding #11674 (similar but distinct): Both affect exec/subsystem callbacks in DoChannelRequest, but #11674 invokes callbacks after failed parsing; this candidate dereferences channel->command after a successful-path callback may free the channel. The root causes and faulting operations differ, so parse-success gating would not fix this lifetime bug.
Suggested fix: Move command logging before the callback, or reacquire the channel and log only when it still exists.
Basis: ISO C17 §7.22.3.3: accessing storage after it has been deallocated by free has undefined behavior.
Referenced code: src/internal.c:13153-13157 (5 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSH_SFTP_accept() serves an application-driven session only on a channel whose subsystem request was answered CHANNEL_SUCCESS. DoChannelRequest() records the session type and command before it decides, and leaves both set on a refusal, so they cannot say by themselves whether anything was granted. - add channel->sessionGranted, set from the answer a shell, exec or subsystem request gets rather than from the request arriving - look the channel up again before recording it: a callback may close its own channel, and wolfSSH_ChannelFree() frees it - log a request's strings where they are known good: once the parse has succeeded, and ahead of a callback that may free the channel - gate the app-channels path on that flag alongside the session type and the command - cover a refusal from both sides, no callback registered and a callback that rejects, and a callback that frees its channel
wolfSSH_SFTP_accept() serves an application-driven session only on a channel the subsystem callback granted sftp on, and compared just the first four bytes of the name, so a granted "sftpx" was served the built-in server. Compare the whole name. - cover a granted subsystem whose name only starts with sftp
CheckSftpAcceptRefusesUngranted() drives the same refusal two ways, a registered subsystem callback saying no and app channels standing in for a missing one, and asserted nothing that told them apart. Assert the call count each case expects.
f8d3591 to
fb9e635
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-src, wolfssh-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
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.