logout user in the case that the underlying connection gets disconnected by JacobBarthelmeh · Pull Request #383 · wolfSSL/wolfHSM

JacobBarthelmeh · 2026-05-28T04:04:08Z

Follow up to item 4 from #270

Copilot

Pull request overview

This PR ensures that an authenticated user session is properly logged out when the underlying communication channel is abruptly disconnected (not just on a clean COMM_CLOSE). It also defensively clears any stale auth session state during wh_Server_Init, in case the externally-owned auth context carries a leftover session from a prior connection.

Changes:

In wh_Server_SetConnected, invoke wh_Auth_Logout on the transition to WH_COMM_DISCONNECTED when a user is currently active.
In wh_Server_Init, after binding the externally-owned auth context, log out any stale active user; fall back to zeroing the user struct if logout fails.
Add _whTest_Auth_AbruptDisconnect test that uses a wrapping test_Logout callback to verify the disconnect path triggers exactly one logout and that repeated disconnects are no-ops.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/wh_server.c	Adds logout-on-disconnect in `wh_Server_SetConnected` and stale-session cleanup in `wh_Server_Init`.
test/wh_test_auth.c	Adds a logout-counting callback and a memory-transport test verifying logout fires on abrupt disconnect and is idempotent.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

bigbrett

LGTM. A few questions and a nit but nothing that would block merge

billphipps · 2026-06-10T15:01:12Z

+         * (mirrors the defensive clear in wh_Server_Init). */
+        if (logout_rc != WH_ERROR_OK ||
+            server->auth->user.user_id != WH_USER_ID_INVALID) {
+            memset(&server->auth->user, 0, sizeof(server->auth->user));


Consider unconditionally zeroing this. Also above too. Losing privileges should be no-fail, just like destructors. Gaining privileges is a must-pass.

Added wh_Auth_Reset for an internal to wolfHSM user reset function and called it unconditionally. It also tries to make use of the lock when available before calling memset.

Also updated the if statement so that wh_Auth_Reset is always called, not just when the user.user_id is set. https://github.com/wolfSSL/wolfHSM/pull/383/changes#diff-a350c6f23e21d6aec09b776df6f81fe9c22ad4aab574292aefb99d0579acf76fR92-R93

billphipps

Please consider unconditionally zeroing userid when logging out. Please resolve additional comments.

JacobBarthelmeh · 2026-06-10T17:58:52Z

@bigbrett and @billphipps , I believe I've addressed the current feedback. Thank you for reviewing

bigbrett · 2026-06-10T21:29:30Z

+    }
+
+    /* Best-effort lock, but clear the session regardless */
+    rc = WH_AUTH_LOCK(context);


@JacobBarthelmeh Seems odd to still clear the session if you can't acquire the lock - this means someone else could be in process of reading. Perhaps the backend would get put into a weird state if synchronous access isn't respected? I'd rather just hard fail if you can't acquire the lock, and let the caller deal with it since it probably means their entire system is broken? I guess you could make the argument that the locking function really shouldn't ever fail, but kind of a bucket of worms. Thoughts?

I'm okay with changing it to be a hard fail when the lock fails. I think that is better behavior than what I'd added for continuing to memset anyway.

@JacobBarthelmeh sounds good, pls assign me when ready for re-review

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #383

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #383

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot · 2026-06-26T23:43:40Z

+        if (rc != WH_ERROR_OK) {
+            WH_LOG(&server->log, WH_LOG_LEVEL_SECEVENT,
+                   "Failed to clear auth session during server init");
+            return rc;


🔵 [Low] Auth-reset failure in wh_Server_Init returns without wh_Server_Cleanup, leaking the log backend · Resource leaks on error paths

When wh_Auth_Reset fails, wh_Server_Init returns rc directly. By this point wh_Log_Init has already initialized the log backend (line 99), and the function's two other error paths both call wh_Server_Cleanup (which runs wh_Log_Cleanup). This early return skips it, leaking the log backend.

Fix: Call (void)wh_Server_Cleanup(server); before returning on the auth-reset failure path, matching the function's other error returns.

wolfSSL-Fenrir-bot · 2026-06-26T23:43:40Z

+    /* Log out any active user on disconnect, including abrupt drops where
+     * COMM_CLOSE never arrives. */
+    if (connected == WH_COMM_DISCONNECTED &&
+        server->connected != WH_COMM_DISCONNECTED &&


🟠 [Medium] Graceful COMM_CLOSE no longer logs out for transports that don't track connected state · Logic errors

The COMM_CLOSE handler now defers logout to wh_Server_SetConnected, gated on server->connected != WH_COMM_DISCONNECTED. Transports whose connect callback never sets CONNECTED (shared-memory, whose connectcb is a no-op) leave server->connected at its zeroed WH_COMM_DISCONNECTED default, so a logged-in user is never cleared on graceful close.

Fix: Track CONNECTED state for all transports, or have the close handler force logout/reset regardless of the prior connected state.

JacobBarthelmeh self-assigned this May 28, 2026

Copilot AI review requested due to automatic review settings May 28, 2026 04:04

Copilot started reviewing on behalf of JacobBarthelmeh May 28, 2026 04:04 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

JacobBarthelmeh force-pushed the auth_connection branch from d2a889c to 5985499 Compare May 28, 2026 22:41

logout user in the case that the underlying connection gets disconnected

1109abb

JacobBarthelmeh force-pushed the auth_connection branch from 5985499 to 1109abb Compare May 29, 2026 21:16

JacobBarthelmeh requested a review from Copilot May 29, 2026 21:16

Copilot started reviewing on behalf of JacobBarthelmeh May 29, 2026 21:16 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

JacobBarthelmeh assigned bigbrett and wolfSSL-Bot and unassigned JacobBarthelmeh May 29, 2026

JacobBarthelmeh marked this pull request as ready for review May 29, 2026 21:29

bigbrett assigned rizlik and bigbrett and unassigned bigbrett and wolfSSL-Bot Jun 9, 2026

bigbrett previously approved these changes Jun 10, 2026

View reviewed changes

Comment thread src/wh_server.c Outdated

Comment thread src/wh_server.c Outdated

Comment thread test/wh_test_auth.c Outdated

bigbrett assigned billphipps and unassigned bigbrett Jun 10, 2026

billphipps reviewed Jun 10, 2026

View reviewed changes

billphipps previously approved these changes Jun 10, 2026

View reviewed changes

add wh_Auth_Reset to force user reset and add logging messages

15cb710

JacobBarthelmeh dismissed stale reviews from billphipps and bigbrett via 15cb710 June 10, 2026 15:58

bigbrett reviewed Jun 10, 2026

View reviewed changes

fail closed now when lock fails with wh_Auth_Reset

f19487e

bigbrett unassigned rizlik Jun 11, 2026

bigbrett assigned JacobBarthelmeh and unassigned billphipps Jun 11, 2026

JacobBarthelmeh assigned bigbrett and unassigned JacobBarthelmeh Jun 11, 2026

bigbrett requested a review from wolfSSL-Fenrir-bot June 17, 2026 20:25

wolfSSL-Fenrir-bot reviewed Jun 17, 2026

View reviewed changes

Comment thread src/wh_server.c

bigbrett reviewed Jun 18, 2026

View reviewed changes

Comment thread src/wh_server.c

bigbrett assigned JacobBarthelmeh and unassigned bigbrett Jun 18, 2026

move auth checks to after logging init so that logs outputs can work

a62dac5

JacobBarthelmeh requested a review from wolfSSL-Fenrir-bot June 26, 2026 23:17

wolfSSL-Fenrir-bot reviewed Jun 26, 2026

View reviewed changes

Uh oh!

Conversation

JacobBarthelmeh commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billphipps left a comment

Choose a reason for hiding this comment

Uh oh!

JacobBarthelmeh commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #383

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #383

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JacobBarthelmeh commented May 28, 2026 •

edited

Loading