rekey: trigger highwater on per-key packet count#963
rekey: trigger highwater on per-key packet count#963ejohnstown wants to merge 1 commit intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds per-key-epoch packet counters and a configurable packet-count “highwater” threshold to trigger the existing highwater callback (typically causing a rekey), aligning with RFC 4344 guidance on limiting packets per key.
Changes:
- Track
txMsgCount/rxMsgCountper key epoch and reset them onNEWKEYS. - Extend
HighwaterCheck()to also fire the highwater callback when the per-key packet count reachesmsgHighwaterMark(default2^31). - Add public APIs to set/get the message highwater mark:
wolfSSH_CTX_SetMsgHighwater(),wolfSSH_SetMsgHighwater(),wolfSSH_GetMsgHighwater().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wolfssh/ssh.h | Adds public API declarations for configuring per-key packet-count highwater marks. |
| wolfssh/internal.h | Adds msg-highwater defaults and new per-key packet counters/flags to internal structs. |
| src/ssh.c | Implements the new msg-highwater set/get APIs. |
| src/internal.c | Integrates msg-count checks into HighwaterCheck() and resets/increments msg counters across key epochs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Track txMsgCount/rxMsgCount per key epoch and reset on NEW_KEYS; seq/peerSeq still wrap freely per RFC 4253 Sec 6.4. - Extend HighwaterCheck to fire highwaterCb when packet count crosses msgHighwaterMark (default 2^31, RFC 4344 Sec 3.1). - Add wolfSSH_CTX_SetMsgHighwater / SetMsgHighwater / GetMsgHighwater. Issue: F-246
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #963
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
| ssh->rxMsgCount >= ssh->msgHighwaterMark)) { | ||
|
|
||
| WLOG(WS_LOG_DEBUG, "%s over msg high water mark", | ||
| (side == WOLFSSH_HWSIDE_TRANSMIT) ? "Transmit" : "Receive"); |
There was a problem hiding this comment.
side is only used for the log in this func. Seems the callback will be called even if side is not what the caller specified. Not sure if this is a problem
| /* packet count high water mark functions (RFC 4344 Sec 3.1) */ | ||
| WOLFSSH_API int wolfSSH_CTX_SetMsgHighwater(WOLFSSH_CTX* ctx, word32 highwater); | ||
| WOLFSSH_API int wolfSSH_SetMsgHighwater(WOLFSSH* ssh, word32 highwater); | ||
| WOLFSSH_API word32 wolfSSH_GetMsgHighwater(WOLFSSH* ssh); |
There was a problem hiding this comment.
would it make sense to have a symmetric wolfSSH_CTX_GetMsgHighwater?
|
can you add test cases for the new public api's? |
Issue: F-246