agent: let the application open the agent channel - #1230
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new pollable API can return WS_SUCCESS on retry while output is still pending (masking WS_WANT_*), and the header comment currently understates the actual error/return surface.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes server-side SSH agent forwarding usable for applications that manage channel opens themselves (i.e., don’t follow the default wolfSSH_accept() channel flow), by factoring the auth-agent channel open logic into a callable API.
Changes:
- Adds a new public API
wolfSSH_AGENT_ChannelOpen()for server-side opening ofauth-agent@openssh.comonce the peer requested it. - Updates
wolfSSH_accept()to call the new API instead of embedding the channel-open logic inline. - Adjusts agent-channel open behavior to be idempotent and to “publish” the agent even when the open is queued due to
WS_WANT_WRITE/WS_WANT_READ.
File summaries
| File | Description |
|---|---|
| wolfssh/agent.h | Declares the new server-side agent-channel open API and documents intended behavior. |
| src/ssh.c | Switches wolfSSH_accept() to call the new helper API for opening the agent channel. |
| src/agent.c | Implements wolfSSH_AGENT_ChannelOpen() and updates queuing/idempotency behavior around channel open retries. |
Review details
- Files reviewed: 3/3 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.
96e3b80 to
7010bf4
Compare
7010bf4 to
7b97ef3
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1230
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.
7b97ef3 to
670ee14
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1230
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 #1230
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.
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 on a client session, and is idempotent after, 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 - flush what is left of a queued open on the next call, rather than reporting a success the peer never saw - record ssh->error from the send alone, so neither a poll ahead of the request nor a failed allocation stops accept() continuing
wolfSSH_AGENT_ChannelOpen() answers a poll on a session that is over with WS_FATAL_ERROR and WS_DISCONNECT in ssh->error, the shape every other public sender uses: no channel opened, nothing on the wire, RFC 4253 section 11.1. wolfSSH_accept() gates the open it drives, so the new public entry point is the only way in. - promote SendAfterDisconnect() to WOLFSSH_LOCAL so agent.c uses the same helper as every other public sender - leave an open queued before the disconnect unflushed, the rule wolfSSH_shutdown() applies to all but its own disconnect - keep WS_DISCONNECT in ssh->error at the accept() call site, which used to overwrite it with the status the open returns
00448b1 to
d166aa8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1230
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 #1230
Scan targets checked: wolfssh-bugs, wolfssh-src
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.
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.
which still calls it.
afterward, so an application can poll it.
does not open a second channel and leak the first.