internal: commit a session only once accepted - #1235
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
A shell, exec or subsystem request changes the channel only once the callback accepts it. The session type and command are set for the callback to read and put back if it refuses, and CLIENT_DONE follows acceptance alone, so wolfSSH_accept() no longer reports an established session, or starts SFTP, on a request it answered CHANNEL_FAILURE. - DoChannelRequestSession() carries the three arms, which differed only in the type and the callback consulted - unit.c drives a refused shell, exec and subsystem request through DoChannelRequest() and checks nothing was committed - regress.c runs a server whose shell callback refuses and checks accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT Issue: F-8852
There was a problem hiding this comment.
🟡 Changes recommended
wolfSSH_AGENT_ChannelOpen() currently contradicts its “safe to poll” contract by returning WS_BAD_ARGUMENT in the pre-request state (and wolfSSH_SFTP_accept()’s appChannels gating can advance SFTP without a properly established subsystem), which can lead to incorrect/fragile runtime behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refines server-side channel/session handling so a shell/exec/subsystem request only “commits” the session (and advances accept/session state) after the relevant callback accepts it, preventing wolfSSH_accept() (and related entry points like SFTP) from reporting an established session after sending CHANNEL_FAILURE.
Changes:
- Consolidates shell/exec/subsystem request handling into
DoChannelRequestSession()and defers committingsessionType/command+CLIENT_DONEuntil acceptance. - Introduces “application-driven channels” mode (
wolfSSH_{CTX_,}SetAppChannels) that stopswolfSSH_accept()after userauth. - Adds
wolfSSH_AGENT_ChannelOpen()so app-driven servers can open the agent-forwarding channel afteraccept()returns early; adds unit/regression coverage for the new behaviors.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents and exposes application-driven channel mode APIs. |
| wolfssh/internal.h | Adds appChannels flag to context/session internal structs. |
| wolfssh/agent.h | Exposes wolfSSH_AGENT_ChannelOpen() for app-driven servers. |
| tests/unit.c | Adds unit coverage for rejected session requests and appChannels “no callback” behavior. |
| tests/regress.c | Adds regression coverage for accept stopping at userauth, callback/no-callback behavior, and rejected session requests not advancing accept state. |
| src/wolfsftp.c | Adjusts SFTP accept preconditions when appChannels is enabled. |
| src/ssh.c | Updates wolfSSH_accept() to stop at userauth in appChannels mode and factors agent channel opening into a helper. |
| src/internal.c | Adds DoChannelRequestSession() and updates channel request processing to only commit on acceptance. |
| src/agent.c | Implements wolfSSH_AGENT_ChannelOpen() helper. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| else if (!ssh->useAgent) { | ||
| /* Nothing asked for agent forwarding on this session. */ | ||
| ret = WS_BAD_ARGUMENT; | ||
| } |
| /* check accept is done, if not call wolfSSH accept. In | ||
| * 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 | ||
| && ssh->acceptState >= ACCEPT_SERVER_USERAUTH_SENT)) { |
| /* 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 |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1235
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.
| } | ||
|
|
||
| if (ssh != NULL) | ||
| ssh->error = ret; |
There was a problem hiding this comment.
wolfSSH_AGENT_ChannelOpen() latches WS_BAD_ARGUMENT into ssh-error on its documented polling return · Incorrect error handling
ssh->error is assigned unconditionally, so the "peer has not asked for agent forwarding yet" return (WS_BAD_ARGUMENT, agent.c:1746) — which agent.h:188 documents as safe to poll — latches a fatal error on a healthy session. wolfSSH_accept() then returns WS_INVALID_STATE_E (ssh.c:598) and wolfSSH_get_error() reports the stale failure. The success path likewise clears a previously latched error.
Fix: Assign ssh->error only for genuine failures, leaving it untouched for the not-yet-requested and success returns.
Stacked on #1233, whose three commits are the first here; review the last
one. A shell, exec or subsystem request changes the channel only once the
callback accepts it, so wolfSSH_accept() no longer reports an established
session, or starts SFTP, on a request it answered CHANNEL_FAILURE.
in the type and the callback consulted.
back if it refuses, and CLIENT_DONE follows acceptance alone.
checks accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT.
Issue: F-8852