ssh: add generic request callbacks - #1236
ejohnstown wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness/security issues around silently truncated request names/types being passed to the new generic callbacks (and a readiness-state bug in wolfSSH_SFTP_accept() under app-driven mode) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds “generic” (any-name) callbacks for SSH channel requests and global requests, enabling centralized policy decisions (accept/reject/unhandled) before the existing type-specific callbacks and built-in handling. It also introduces an application-driven channel mode that stops wolfSSH_accept() after authentication so applications can drive channel open/request handling via wolfSSH_worker(), with accompanying agent-channel and SFTP adjustments plus expanded unit/regression coverage.
Changes:
- Add
wolfSSH_CTX_SetChannelReqCb()andwolfSSH_CTX_SetGlobalReqCb()to allow first-pass policy decisions for all channel/global requests. - Add
wolfSSH_{CTX_,}SetAppChannels()and update accept/worker flows to support application-driven channel handling. - Add
wolfSSH_AGENT_ChannelOpen()helper and extend tests (unit.c,regress.c) to validate callback interactions and app-driven semantics.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Public API additions: generic request callback types, tri-state result enum, and app-driven channel mode API/docs. |
| wolfssh/internal.h | Extend WOLFSSH_CTX/WOLFSSH with generic callback pointers and appChannels flags. |
| wolfssh/agent.h | Add public API for server-side agent channel opening in app-driven mode. |
| src/ssh.c | Implement new setters and modify wolfSSH_accept() to support stopping at authenticated state when app-driven. |
| src/internal.c | Route channel/global requests through new generic callbacks; refactor session request handling. |
| src/wolfsftp.c | Adjust SFTP accept logic to account for app-driven accept state behavior. |
| src/agent.c | Implement wolfSSH_AGENT_ChannelOpen() and refactor agent channel open path. |
| tests/unit.c | Add unit coverage around session-request rejection and app-driven “no default callback acceptance” behavior. |
| tests/regress.c | Add regression tests for generic callbacks, app-driven accept stopping point, and callback bypass rules. |
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.
d70cae4 to
0c27f30
Compare
0c27f30 to
aaa6148
Compare
aaa6148 to
d29065c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1236
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.
5ce4c71 to
591ea7d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1236
Scan targets checked: wolfssh-src, wolfssh-bugs
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.
wolfSSH_CTX_SetChannelReqAnyCb() and wolfSSH_CTX_SetGlobalReqAnyCb() register a callback consulted first for a channel or global request, with the name, the type-specific part, and whether a reply is wanted. A tri-state answer grants, refuses, or leaves it to the handling already there, so a policy reaches the types with no hook of their own. - the name is the one that arrived, since a copy into a buffer truncates a long name and ends it at an embedded NUL, and a policy has to answer on what the peer sent - a grant still parses and records what the library needs, so a granted session request commits the session, the typed callbacks are not consulted, and a granted unknown type is answered CHANNEL_SUCCESS - a port-0 tcpip-forward skips the callback, since only the forward callback can report the port bound and a policy that had bound a listener would then have to be refused, per RFC 4254 7.1 - a client refuses tcpip-forward and cancel-tcpip-forward ahead of any policy, matched on the name that arrived so it holds in a build with no forwarding, where neither name is in the name table - window-change, exit-status and exit-signal are cleared of a reply before the handling runs, so a refusal leaves them unanswered too, per RFC 4254 6.7 and 6.10 - regress.c covers the answers, the data delivered, the names that do not fit a copy, and which callbacks each answer leaves out
The generic channel request callback may free the channel it was handed, so DoChannelRequest() looks it up again before the type handling reads it. Gone, the request ends there, and a reply the peer wanted fails on the missing channel the way one after a typed session callback does. - ssh.h says the callback may free its channel and what the request does from there - regress.c frees the channel from the callback on each of the three answers, and on a pty-req, the type that wrote to the channel outside a session request
591ea7d to
e0b114e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1236
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
wolfSSH_CTX_SetChannelReqCb()andwolfSSH_CTX_SetGlobalReqCb()register a callback consulted first for every channel and global request, with the name and the type-specific part to parse.