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.
wolfSSH_AGENT_ChannelOpen() settles its channel and agent on whether the open reached the peer, which SendPacketDelivered() answers from the flush count. The highwater callback runs after the last byte is out, so its failure arrives as this send's return, and the rollback read that as a send that never left. - give SendPacketDelivered() external linkage, contract in internal.h - note in agent.h that an error raised once the open is on the wire leaves the channel open - test a failing highwater callback: the channel and the agent stand, and the poll after it is the idempotent one
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.