ssh: add opt-in application-driven channels by ejohnstown · Pull Request #1233 · wolfSSL/wolfssh

ejohnstown · 2026-09-03T00:19:22Z

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. On, accept() returns once the user is authenticated and a
session request with no callback behind it is refused.
Keep the stop state out of the pending-send advance, and stop early
only while the session is short of that state.
Teach wolfSSH_SFTP_accept() that the mode parks accept() short of an
established session, and serve the built-in server only on a channel
the application granted sftp on.
Match the sftp subsystem name whole, by the parsed wire length as well
as the bytes, in that gate and in accept()'s divert (F-11665).
regress.c drives both modes, unit.c checks DoChannelRequest() refuses
an uncallbacked request once the pivot is on.

Copilot

🟡 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 stop wolfSSH_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 into wolfSSH_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.

wolfSSL-Fenrir-bot

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

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

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.

wolfSSL-Fenrir-bot

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

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.

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 - 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

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

wolfSSL-Fenrir-bot

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.

The built-in SFTP server takes a session only when the subsystem name is sftp, matched whole. DoChannelRequest() keeps the parsed length in channel->commandSz, so neither wolfSSH_SFTP_accept()'s grant gate nor wolfSSH_accept()'s divert serves "sftpx" or "sftp\0evil". - cover a granted name longer than sftp, one of its length, and one running past an embedded NUL - cover the divert with those three names and a control that diverts - exec keeps its command length too Issue: F-11665

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.

wolfSSL-Fenrir-bot

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.

An application vetting an exec or subsystem request in its channel request callback is handed the command as a C string, which stops at an embedded NUL. wolfSSH_ChannelGetSessionCommandSz() and wolfSSH_GetSessionCommandSz() report the parsed wire length, so a callback can match a name whole the way DoChannelRequest() does. - both accessors report 0 for a NULL channel or session - wolfSSH_GetSessionCommand() defers to the channel accessor - the sftp divert in wolfSSH_accept() asks the accessor for the length - correct the trace name in wolfSSH_ChannelGetSessionCommand() - cover a callback seeing "sftp\0evil" through exec and subsystem

DoChannelRequest() records the session type and asks the exec and subsystem callbacks whether to grant a session only when the command string parsed. A failed parse is refused on ret alone, and channel->command still holds an earlier request's value rather than the one being answered. - cover a command length header running past the end of the packet, on exec and on subsystem Issue: F-11674

The peer's exec or subsystem command line is wiped ahead of both frees that release it, ChannelDelete() and the GetStringAlloc() that replaces it on a repeat request, the way ChannelDelete() already wipes the decrypted inputBuffer just above. A command line can carry a password or a token among its arguments. - ScrubChannelCommand() leaves the free to GetStringAlloc(), so a parse that fails behind it holds no dangling pointer - cover both wipes with the retain-on-free allocator, the replacement through wolfSSH_TestDoChannelRequest() - release the test's hand-built channel on a setup failure Issue: F-8850

DoChannelRequest() returns early when the header parse fails, so the ret == WS_SUCCESS test that followed it could never be false. The channel lookup moves into the else, which is the only way the function reaches it. Issue: F-11657

wolfSSH_accept() hands a session to the built-in SCP or SFTP server only when the request naming it was answered CHANNEL_SUCCESS. A callback that refuses one sends CHANNEL_FAILURE, yet sessionType and command are recorded ahead of that answer and stay set, so the diverts read a refused session as a served one. - gate both diverts on channel->sessionGranted, as wolfSSH_SFTP_accept() already gates the app-channels path - the SCP divert tests the channel list itself, having no command lookup ahead of it to do that - cover each divert with a refused request and a granted control

issues addressed

Copilot AI lite review requested due to automatic review settings September 3, 2026 00:19

This was referenced Sep 3, 2026

echoserver: answer session requests in callbacks #1234

Merged

internal: commit a session only once accepted #1235

Merged

ejohnstown requested a review from wolfSSL-Fenrir-bot September 3, 2026 00:22

Copilot started reviewing on behalf of ejohnstown September 3, 2026 00:26 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread src/ssh.c

Comment thread wolfssh/agent.h Outdated

ejohnstown self-assigned this Sep 3, 2026

ejohnstown force-pushed the ccb-phase2-4d branch from 8121dac to 127b742 Compare September 4, 2026 05:22

ejohnstown requested review from wolfSSL-Fenrir-bot and removed request for wolfSSL-Fenrir-bot September 4, 2026 05:22

wolfSSL-Fenrir-bot requested changes Sep 4, 2026

View reviewed changes

Comment thread src/agent.c

Comment thread src/wolfsftp.c Outdated

ejohnstown force-pushed the ccb-phase2-4d branch 2 times, most recently from f359ec4 to ab06a5f Compare September 8, 2026 18:28