Skip to content

fix: always send USERAUTH_FAILURE on reject - #1202

Open
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:sf23
Open

fix: always send USERAUTH_FAILURE on reject#1202
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:sf23

Conversation

@ejohnstown

@ejohnstown ejohnstown commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

RFC 4252 section 5.1 has the server answer a request it does not accept with USERAUTH_FAILURE. A callback returning WOLFSSH_USERAUTH_REJECTED now always gets that reply.

NO_FAILURE_ON_REJECTED suppressed it. The macro was never set by configure, named in a header, or documented, and it guarded all four method handlers alike.

  • drop the macro and its four guards
  • add test_UserAuthRejectedSendsFailure(), asserting on the wire, since the handler returns WS_USER_AUTH_E either way

Issue: F-11672

Copilot AI lite review requested due to automatic review settings August 27, 2026 19:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates wolfSSH’s server-side user authentication handling to comply with RFC 4252 §5.1 by ensuring a rejected authentication attempt still results in a USERAUTH_FAILURE reply on the wire, and adds a unit test that asserts this behavior.

Changes:

  • Removed NO_FAILURE_ON_REJECTED guards so WOLFSSH_USERAUTH_REJECTED consistently triggers USERAUTH_FAILURE sending across auth methods.
  • Added test_UserAuthRejectedSendsFailure() to validate the on-the-wire response when the userauth callback rejects.
  • Wired the new test into the unit test runner.

Reviewed changes

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

File Description
src/internal.c Always sends USERAUTH_FAILURE when userauth callback returns WOLFSSH_USERAUTH_REJECTED (removes macro guards).
tests/unit.c Adds a unit test that captures outgoing packets and asserts MSGID_USERAUTH_FAILURE is sent on callback rejection.
Suppressed comments (1)

src/internal.c:8756

  • Setting authFailure=1 on WOLFSSH_USERAUTH_REJECTED will cause SendUserAuthFailureCount() to run later, but the function currently forces ret back to WS_USER_AUTH_E when authRejected is set. That can mask a WS_WANT_WRITE from SendUserAuthFailureCount(), despite nearby comments stating a blocked send is non-fatal.
                else if (ret == WOLFSSH_USERAUTH_REJECTED) {
                    WLOG(WS_LOG_DEBUG, "DUARPW: password rejected");
                    authFailure = 1;
                    authRejected = 1;
                    ret = WS_USER_AUTH_E;
                }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/internal.c
Comment thread src/internal.c
Comment thread src/internal.c
@ejohnstown
ejohnstown force-pushed the sf23 branch 2 times, most recently from 7853fc6 to 09f3a40 Compare August 27, 2026 20:51

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1202

Scan targets checked: wolfssh-bugs, wolfssh-src

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

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

Comment thread tests/unit.c
Comment thread tests/unit.c
Comment thread tests/unit.c
Comment thread tests/unit.c
Comment thread tests/unit.c
Comment thread tests/unit.c
@ejohnstown
ejohnstown requested review from wolfSSL-Fenrir-bot and removed request for wolfSSL-Fenrir-bot August 27, 2026 23:33

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1202

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.

@wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot dismissed their stale review August 28, 2026 17:29

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

@ejohnstown
ejohnstown requested a review from philljj August 28, 2026 22:30
@philljj philljj self-assigned this Aug 28, 2026

@philljj philljj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflict in unit.c, and skoll High.

Comment thread src/internal.c
@philljj philljj assigned ejohnstown and unassigned philljj Aug 28, 2026
RFC 4252 section 5.1 has the server answer a request it does not accept
with USERAUTH_FAILURE. A callback returning WOLFSSH_USERAUTH_REJECTED
now always gets that reply, and always ends the session.

NO_FAILURE_ON_REJECTED suppressed the reply. The macro was never set by
configure, named in a header, or documented, and it guarded all four
method handlers alike.

- drop the macro and its four guards
- end the session on a rejected keyboard-interactive setup, the one
  rejection that used to leave the peer retrying to the cap
- state the guarantee on WOLFSSH_USERAUTH_REJECTED in wolfssh/ssh.h
- add test_UserAuthRejectedSendsFailure() over the dispatched methods,
  asserting on the wire since the handlers return WS_USER_AUTH_E anyway

Issue: F-11672
@ejohnstown

Copy link
Copy Markdown
Contributor Author

Rebased onto master, so the unit.c conflict is gone. The two versions of
the request builder are merged: it takes the service name, and keeps the
guard the publickey case needs.

The skoll High is answered on its thread -- real, but not new to this PR,
and the fix belongs in the teardown flush, which has to follow the channel
EOF/close PR in flight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants