agent, apps, examples: relay whole agent messages

[yosuke-wolfssl]

Problem

Agent forwarding relays a length-prefixed message protocol across two byte
streams — the SSH channel and the local ssh-agent socket — and neither side
respected message boundaries.

readPeer() in apps/wolfssh/wolfssh.c and examples/client/client.c read
into a 512-byte stack buffer, did one top-up read, then relayed whatever
they had. wolfSSH_AGENT_Relay() did a single agentIoCb write whose short
write was reported as success, and a single read declared a complete reply.

A remote server controls how its agent requests are sized and fragmented, so
one over 508 bytes was truncated, and one split across three or more
deliveries could never be assembled — malformed messages reached the local
agent and desynced the agent protocol. Five defects in all: no size check, no
reassembly state across calls, a negative read return folded into a length,
trailing bytes of the next message counted into the relayed length, and an
unchecked wolfSSH_GetLastRxId().

Fix (src/agent.c)

Framing moves into the library.

• AgentWriteAll(), AgentReadFull(), AgentReadMessage() loop the
  agentIoCb and read a length prefix bounded by a new
  WOLFSSH_AGENT_MAX_MSG_SZ (256 KB, overridable). AgentWriteMessage()
  reconnects once when a write finds the agent socket dead.
  wolfSSH_AGENT_Relay() is rebuilt on them with its signature unchanged, so
  integrators inherit the read fix without changing code.
• New wolfSSH_AGENT_RelayChannel() frames the channel side, holding a
  partial request and an unfinished reply on WOLFSSH_AGENT_CTX between
  calls. Its return names what holds an owed reply:

Return	Meaning
WS_SUCCESS	nothing owed
WS_WANT_WRITE	the transport holds it
WS_WINDOW_FULL, WS_REKEYING	the peer holds it

It also drives the transport when a held status leaves bytes queued below —
SendChannelData() returns WS_REKEYING ahead of its own flush — and then
answers WS_WANT_WRITE, so the caller waits on the socket that owns them.

Both readPeer() copies collapse to wolfSSH_GetLastRxId() +
wolfSSH_AGENT_RelayChannel(), dropping their local ato32() and 512-byte
buffer, and arm a select() write set only for the transport's own
want-write so a full peer window cannot spin the read thread.

Closes f-10545.

Tests

Nine cases for wolfSSH_AGENT_Relay() in tests/api.c and twenty-two for
wolfSSH_AGENT_RelayChannel() in tests/regress.c, covering fragmentation,
trailing bytes, oversize and zero lengths, short writes, agent reconnect,
buffer scrubbing, and each reason a reply stays owed.

Verification

• make check 13/13.
• GCC-13 -Werror clean across all six preflight configs.
• ASan + UBSan clean.
• Mutation-tested: every fix has a negative control that fails without it.

Not in this PR

SendChannelData() returns the full dataSz when its second pre-send flush
(src/internal.c:23207) leaves output queued, having framed nothing — the
first flush returns early on that, the second falls through to
if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) ret = dataSz;. Every caller
reads a count for bytes that reached no buffer, wolfSSH_stream_send()
included, so it is raised on its own rather than worked around here.

The relay reaches it when a channel read defers a window adjust
(outputBuffer.length != 0 with plainSz == 0) and the socket is still full
at the reply send: the reply is dropped and reported as delivered.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues remain in channel routing, retry handling, output flushing, message validation, and reply-delivery accounting.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR moves SSH-agent framing and relay state into the library, updates client integrations, and expands test coverage.

Changes:

• Adds bounded full-message I/O, reconnect handling, and stateful channel relaying.
• Updates the CLI and example client integrations.
• Adds API and regression tests for fragmentation, retries, limits, and backpressure.

File summaries

File	Summary
wolfssh/agent.h	Exposes relay state and the channel relay API.
tests/regress.c	Tests channel fragmentation, retries, and reply backpressure.
tests/api.c	Tests framed relay behavior, limits, short writes, and reconnects.
src/agent.c	Implements framing, buffering, reconnects, and channel relaying; unresolved review findings remain.
examples/client/client.c	Integrates channel relaying; unresolved routing and retry issues remain.
apps/wolfssh/wolfssh.c	Integrates channel relaying; unresolved routing and retry issues remain.

Review details

Suppressed comments (7)

apps/wolfssh/wolfssh.c:726

• When this call returns WS_REKEYING, AgentRelayHeld() correctly leaves agentOwed set, but the code then continues into wolfSSH_stream_read(). In this read loop a WS_FATAL_ERROR with wolfSSH_get_error() == WS_REKEYING falls through to err_sys() (unlike examples/client/client.c:567-570), so a valid held reply during a rekey terminates the app instead of driving and retrying the rekey. Handle WS_REKEYING in this loop before treating the stream read failure as fatal.

                        ret = wolfSSH_AGENT_RelayChannel(args->ssh, channel);
                        agentChannel = channel;
                        agentOwed = AgentRelayHeld(ret, &agentWantsWrite);

apps/wolfssh/wolfssh.c:684

• readPeer() is also governed by while (ret >= 0). A held retry result is negative, so this assignment leaves the loop condition false and abandons the owed reply instead of waiting for the transport/window/rekey to clear. Reset ret to WS_SUCCESS whenever agentOwed is true.

            agentOwed = AgentRelayHeld(relayRet, &agentWantsWrite);

examples/client/client.c:510

• readPeer() exits on the outer while (ret >= 0) condition. When this retry returns a held status (all are negative), agentOwed remains true but ret stays negative, so the thread leaves immediately and never retries the reply. Reset ret to WS_SUCCESS for held statuses while retaining agentOwed.

            agentOwed = AgentRelayHeld(relayRet, &agentWantsWrite);

examples/client/client.c:553

• The first relay call has the same outer-loop failure: WS_WANT_WRITE, WS_WINDOW_FULL, and WS_REKEYING are negative, so leaving the code in ret makes while (ret >= 0) terminate after this iteration even though agentOwed says the reply must be retried. Normalize held results to WS_SUCCESS before continuing.

                            agentOwed = AgentRelayHeld(ret, &agentWantsWrite);

src/agent.c:2259

• wolfSSH_ChannelIdRead() can return bytes successfully while leaving a WINDOW_ADJUST queued and ssh->error set to WS_WANT_WRITE. If this call only accumulates a partial request, no reply is present for AgentRelaySendReply() to flush, so this path returns WS_SUCCESS and the applications disarm their write set; on a nonblocking transport the queued window adjust is then never flushed and the peer can stall. Propagate a pending output/WS_WANT_WRITE status before returning success even when no agent reply is owed.

            break;
    }

    if (ret != WS_SUCCESS && ret != WS_WANT_WRITE && ret != WS_WINDOW_FULL
            && ret != WS_REKEYING && ret != WS_CHAN_RXD && agent != NULL)

src/agent.c:2183

• The new state has one relayBuf and one rxBuf, and this branch clears both when channelId changes. Both updated readPeer() loops call wolfSSH_AGENT_RelayChannel() for every WS_CHAN_RXD, including shell or other channel traffic while an agent request/reply is pending; that silently drops the partial request or owed reply and desynchronizes the agent protocol. Defer unrelated channel events or make the relay state per channel before using this API in those loops.

        if (!agent->relayActive || agent->relayChannel != channelId) {
            AgentBufferReset(agent->relayBuf);
            AgentBufferReset(agent->rxBuf);

src/agent.c:2073

• wolfSSH_AGENT_Relay() passes the caller-provided msgSz directly to AgentWriteMessage() without checking the framing. For *msgSz == 0, AgentWriteAll()'s empty loop returns WS_SUCCESS, so Relay() reads a reply without sending a request; a four-byte zero or oversized header is also forwarded. Validate exactly one non-empty length-prefixed message against WOLFSSH_AGENT_MAX_MSG_SZ before writing, as RelayChannel() does.

        ret = AgentWriteMessage(ssh, msg, *msgSz);

• Files reviewed: 6/6 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.

[Copilot]

🟡 Changes recommended

Critical relay progress and pending-reply handling issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/agent.c:2216

• AgentBufferPrep() passes 512 to GrowBuffer() on every loop, but GrowBuffer() treats that argument as additional free capacity. With one-byte channel fragments, once the partial message is within 512 bytes of capacity this reallocates and copies the entire accumulated message for each byte (up to roughly 512 reallocations for a maximum-sized message), making attacker-controlled fragmentation disproportionately expensive. Grow geometrically or only expand when the current capacity is exhausted.

        ret = AgentBufferPrep(agent, &agent->relayBuf,
                WOLFSSH_AGENT_RELAY_CHUNK_SZ);

src/agent.c:2228

• A positive wolfSSH_ChannelIdRead() result is not sufficient here: _ChannelRead() deliberately returns the byte count even when its window-adjust send fails and records that failure in ssh->error. If this read only completes a partial request, no reply is queued, so AgentRelaySendReply() can return success and hide errors such as WS_SOCKET_ERROR_E or WS_OVERFLOW_E; callers then continue driving a broken channel. Propagate the post-read non-transient transport error (while preserving the documented transient wants) before reporting success.

        else if (rxd > 0) {
            in->length += (word32)rxd;
            progress = 1;
        }

wolfssh/agent.h:216

• The return contract here does not match AgentRelaySendReply(): after a positive partial send leaves bytes in rxBuf, the implementation returns WS_WANT_WRITE whenever the reply tail remains (src/agent.c:2151-2154), even if that send has just reduced peerWindowSz to zero; the regression test expects exactly that at tests/regress.c:3737-3746. Consumers therefore arm write readiness for a reply actually held by the peer. Either return WS_WINDOW_FULL at that point or document WS_WANT_WRITE as a generic retry status rather than transport-only.

 * same channelId must come back for the rest of either. While a reply is
 * owed it names what holds it: WS_WANT_WRITE the transport, WS_WINDOW_FULL
 * or WS_REKEYING the peer. Call again until WS_SUCCESS. Any other

• Files reviewed: 6/6 changed files
• Comments generated: 9
• Review effort level: Lite

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1262

Scan targets checked: wolfssh-src, wolfssh-bugs

Findings: 4
4 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]

Fenrir Automated Review — PR #1262

Scan targets checked: wolfssh-src, wolfssh-bugs

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

💬 2 finding(s) from an earlier review are still open and were not re-posted:

• Switching agent channels discards relay state — src/agent.c:2190
• Agent reconnect leaks the previous socket — src/agent.c:1951

This review was generated automatically by Fenrir. Reported findings require changes before merge.