Skip to content

ssh: add opt-in application-driven channels - #1233

Open
ejohnstown wants to merge 5 commits into
wolfSSL:masterfrom
ejohnstown:ccb-phase2-4d
Open

ssh: add opt-in application-driven channels#1233
ejohnstown wants to merge 5 commits into
wolfSSL:masterfrom
ejohnstown:ccb-phase2-4d

Conversation

@ejohnstown

@ejohnstown ejohnstown commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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.

  • 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, so it stops redoing the handshake on every poll.
  • regress.c drives both modes, unit.c checks DoChannelRequest() refuses
    an uncallbacked request once the pivot is on.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/ssh.c
Comment thread wolfssh/agent.h Outdated
@ejohnstown ejohnstown self-assigned this Sep 3, 2026
@ejohnstown
ejohnstown requested review from wolfSSL-Fenrir-bot and removed request for wolfSSL-Fenrir-bot September 4, 2026 05:22

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/agent.c
Comment thread src/wolfsftp.c Outdated
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
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 and ssh->error alone.

- gate the app-channels branch on wolfSSH_GetSessionType() and
  wolfSSH_GetSessionCommand(), the same test accept() makes
- 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 and on a granted shell,
  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
- 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
Comment thread src/agent.c
Comment thread src/wolfsftp.c Outdated
@ejohnstown
ejohnstown marked this pull request as ready for review September 8, 2026 18:45

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/wolfsftp.c
if (channel == NULL || !channel->sessionGranted
|| channel->sessionType != WOLFSSH_SESSION_SUBSYSTEM
|| channel->command == NULL
|| WSTRNCMP(channel->command, "sftp", 4) != 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SFTP gate accepts subsystem-name prefixes · Logic errors

WSTRNCMP(..., 4) accepts granted names such as sftpx, so built-in SFTP starts without an exact sftp grant. Adjacent to known #8852, this is the new app-mode name predicate, not default accept() ignoring callback rejection.

Suggested fix: Require an exact subsystem-name match, including its full length, before entering the SFTP state machine.
Basis: wolfssh/ssh.h application-driven channel contract: wolfSSH_SFTP_accept() serves only a channel whose callback granted sftp.

Comment thread tests/regress.c
RepointHarnessInput(&harness, in, inSz);
AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS);
/* Either way the peer is told the subsystem was refused. */
AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejected-callback test never verifies callback execution · Weak or missing assertions

CheckSftpAcceptRefusesUngranted(1) never checks sessionReqCbCalls, so it passes if app-channel mode skips the registered callback and merely takes the no-callback rejection path.

Suggested fix: Assert sessionReqCbCalls == 1 in the rejectVia != 0 case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants