ssh: add a rekey-state accessor - #1260
yosuke-wolfssl wants to merge 2 commits into
Conversation
- wolfSSH_RekeyPending() reports whether a key exchange is in flight, returning 0 for a NULL session so a caller may test it directly in a loop condition. - the wolfSSH_worker() block in ssh.h names it as the way to ask, alongside wolfSSH_OutputPending(). - tests/regress.c covers each keying bit alone, both together, and a NULL session for both predicates. - tests/testsuite.c drops its wolfSSH_OutputPending() call; a public function without a WOLFSSH_API prototype fails -Wmissing-prototypes in src/ssh.c, so the call proved nothing the build did not.
There was a problem hiding this comment.
🟢 Approval recommended
The API addition and keying-flag gating change are consistent with existing worker/error semantics, and the PR includes targeted regression/unit coverage for the new behavior and the previously problematic edge case.
Pull request overview
This PR adds a small public API to let consumers query whether a key exchange (rekey) is currently in flight, and fixes a keying-state edge case where SendKexInit() could leave the session stuck “keying” even when the KEXINIT send failed outright.
Changes:
- Add
wolfSSH_RekeyPending(const WOLFSSH*)accessor (NULL-safe, puressh->isKeyingread) and document its intended use alongsidewolfSSH_worker()/wolfSSH_get_error(). - Adjust
SendKexInit()to setWOLFSSH_SELF_IS_KEYINGonly once the KEXINIT packet is successfully sent or queued, avoiding a stale keying flag on outright send failure. - Add regression/unit tests for the accessor and for the “KEX init send fails / short-writes” keying-flag behavior; remove an export-visibility check from
tests/testsuite.c.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents the contract and adds the public wolfSSH_RekeyPending() declaration. |
| wolfssh/internal.h | Updates the semantics comment for WOLFSSH_SELF_IS_KEYING to match the new gating. |
| src/ssh.c | Implements wolfSSH_RekeyPending() as a NULL-safe isKeying predicate. |
| src/internal.c | Moves WOLFSSH_SELF_IS_KEYING set to only after KEXINIT is sent/queued. |
| tests/unit.c | Adds assertions covering predicate behavior and the KEXINIT send-failure/short-write keying gate. |
| tests/testsuite.c | Removes the wolfSSH_OutputPending() export check from the testsuite harness. |
| tests/regress.c | Adds regression coverage for wolfSSH_RekeyPending() over NULL and keying-bit combinations. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1260
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.
- SendKexInit() takes ssh->txFlushCount before wolfSSH_SendPacket() and sets WOLFSSH_SELF_IS_KEYING when SendPacketDelivered() reports the packet away, in place of setting it before the packet is built. - PurgePacket() runs on the same decision, so a packet the transport took is not purged behind an error the highwater callback raised. - internal.h describes the flag as set once the KEX init is sent or queued. - tests/unit.c covers a KEX init whose send fails outright, one that short-writes, and one the transport takes whole behind a highwater callback that fails. - FailHighwater() moves beside the other shared send callbacks and gains WS_MAYBE_UNUSED, so the client test uses it too.
0b337ca to
52bd594
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1260
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.
| if (ctx == NULL) | ||
| return -1897; | ||
| /* No refusals, so the first write resets the socket. */ | ||
| s_sendRefusals = 0; |
There was a problem hiding this comment.
New rekey test breaks client-only builds · Logic errors
test_KexInitSendAwayGatesKeying() is compiled when the client is enabled, but s_sendRefusals and RefuseThenResetIoSend are server-only. Defining NO_WOLFSSH_SERVER makes tests/unit.c fail to compile.
Related known finding #10542 (similar but distinct): Both are build-configuration defects, but #10542 incorrectly gates public-key authentication in DoUserAuthFailure/GetAllowedAuth, whereas this test references server-only symbols in a client-enabled compilation path. The faulting operations, root causes, locations, and required patches differ.
Suggested fix: Move the shared refusal state and callback outside the server-only block, marking the callback unused where necessary.
Basis: ISO C17 §6.5.1 requires identifiers used as primary expressions to designate declared objects or functions.
Problem
Consumers have no way to ask whether a key exchange is running. Across
apps/,examples/,ide/andzephyr/, 24 of 118wolfSSH_get_error()/ssh->errorreads ask exactly that; 21 of them are one repeated pump in
examples/sftpclient/sftpclient.c, which is also the only consumer code in thetree that reads the
WOLFSSHstruct directly.wolfSSH_worker()withholdsWS_REKEYINGwhen its flush failed, so that pump can exit with the rekeyunfinished.
SendKexInit()also setWOLFSSH_SELF_IS_KEYINGbefore it built the packet, soa send that never reached the transport still left the session believing a rekey
was in flight.
Fix (
src/ssh.c,src/internal.c)wolfSSH_RekeyPending(const WOLFSSH*)returns nonzero while a key exchangeis in flight, and 0 otherwise — including for a NULL session, so it is safe as
a loop condition.
ssh->isKeyingread, so it cannot disagree withwolfSSH_worker()'sWS_REKEYINGreturn.SendKexInit()capturesssh->txFlushCountbefore the send and sets theflag when
SendPacketDelivered()reports the packet away. The send's returncannot serve as that signal:
wolfSSH_SendPacket()forwardsHighwaterCheck()'s value, so an application highwater callback can fail withthe KEXINIT already on the wire.
PurgePacket()runs on the same decision.Tests
tests/regress.ctests/unit.cVerification
-Werrorconfigs.unit167 (baseline 166), plusregress,testsuite,kex,apiandauth; thescp,sshclient,get-putandsftpscripts pass serially.place of
SendPacketDelivered(), each fail only the test written for them.Not in this PR
change against a settled contract, not alongside it.
tests/testsuite.closes itswolfSSH_OutputPending()probe. It was theonly such export check among 331 public functions; applying the pattern
consistently would mean adding it to all of them, which is not worth it.