Add an integration test where a client authenticates using public key

[yosuke-wolfssl]

This PR adds the integration test for client authentication scenario to test server-side public key authentication signature verification path.
There are three test functions in tests/auth.c:

• test_pubkey_auth_rsa() — valid RSA key is accepted
• test_pubkey_auth_ecc() — valid ECC key is accepted
• test_pubkey_auth_wrong_key() — RSA-authorized server rejects an ECC key

[Copilot]

Pull request overview

Adds new integration tests that exercise server-side public-key authentication, covering both acceptance (RSA/ECC) and rejection (wrong key) paths.

Changes:

• Adds pubkey auth integration test helpers and three new test cases in tests/auth.c.
• Extends thread_args to support pubkey-test server context and expected-failure behavior.
• Adjusts auth test gating so pubkey tests can run independently of keyboard-interactive and filesystem options.

Reviewed changes

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

File	Description
tests/auth.h	Extends test thread args to carry pubkey server context and expected failure flag.
tests/auth.c	Implements pubkey auth test server/client callbacks, a dedicated server thread, and RSA/ECC/wrong-key integration tests; updates test gating.

---

💡 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 2 comments.

---

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

[dgarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

• [High] Shared functions moved into NO_SHA256-gated section break keyboard-interactive tests — tests/auth.c:157-753
• [Medium] serverPubkeyUserAuth does not check wc_Sha256Hash return value — tests/auth.c:377-378

Review generated by Skoll via openclaw

[dgarske]

🟠 [High] Shared functions moved into NO_SHA256-gated section break keyboard-interactive tests
🚫 BLOCK bug

The PR moved SignalTcpReady (line 403), load_file (line 413), and load_key (line 444) into the new pubkey test guard, which requires !defined(NO_SHA256) (lines 157-159). These functions are also called by server_thread (lines 872, 888) in the keyboard-interactive test section, which has a different guard at lines 755-757:

#if !defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_CLIENT) && \
    !defined(SINGLE_THREADED) && !defined(WOLFSSH_TEST_BLOCK) && \
    !defined(NO_FILESYSTEM) && defined(WOLFSSH_KEYBOARD_INTERACTIVE)

This guard does NOT check NO_SHA256. So when NO_SHA256 is defined (disabling SHA-256) while keyboard-interactive is enabled with a filesystem, the keyboard-interactive section compiles but SignalTcpReady, load_file, and load_key are not defined → compilation failure.

Before this PR, all these functions were under the single keyboard-interactive guard, so NO_SHA256 had no effect on them. This is a regression introduced by the refactoring.

Suggestion:

Suggested change

	#if !defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_CLIENT) && \
	Move `SignalTcpReady`, `load_file`, and `load_key` out of the pubkey guard and into a broader guard that covers both sections:
	```c
	#if !defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_CLIENT) && \
	!defined(SINGLE_THREADED) && !defined(WOLFSSH_TEST_BLOCK)
	static INLINE void SignalTcpReady(tcp_ready* ready, word16 port) { ... }
	#ifndef NO_FILESYSTEM
	static int load_file(...) { ... }
	#endif
	static int load_key(...) { ... }
	#endif

Then start the pubkey-specific section with its !defined(NO_SHA256) guard after these shared helpers.

[yosuke-wolfssl]

Fixed in latest commit

[dgarske]

🟡 [Medium] serverPubkeyUserAuth does not check wc_Sha256Hash return value
💡 SUGGEST bug

In serverPubkeyUserAuth, the return value of wc_Sha256Hash() is not checked. If the hash fails (e.g., FIPS self-test failure, wolfcrypt not initialized), the local hash buffer remains uninitialized, and the subsequent WMEMCMP compares uninitialized stack data against the expected hash. This could lead to undefined behavior.

The echoserver code (line 2302) has the same unchecked pattern, so this is consistent with existing convention, but since this is new code, it's an opportunity to do better.

Suggestion:

Suggested change

	wc_Sha256Hash(authData->sf.publicKey.publicKey,
	if (wc_Sha256Hash(authData->sf.publicKey.publicKey,
	authData->sf.publicKey.publicKeySz, hash) != 0)
	return WOLFSSH_USERAUTH_FAILURE;

[yosuke-wolfssl]

Fixed in latest commit

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/auth.c:1162

• wolfSSH_AuthTest() opens sockets (tcp_listen/connect) but never calls WSTARTTCP(). On USE_WINDOWS_API builds this can fail because WSAStartup isn’t invoked. Add WSTARTTCP() near the start of wolfSSH_AuthTest() (before any socket usage / before wolfSSH_Init()).

int wolfSSH_AuthTest(int argc, char** argv)
{
    (void) argc;
    (void) argv;

#if defined(NO_WOLFSSH_SERVER) || defined(NO_WOLFSSH_CLIENT) || \
    defined(SINGLE_THREADED) || defined(WOLFSSH_TEST_BLOCK)
    return 77;
#else

#if defined(DEBUG_WOLFSSH)
    wolfSSH_Debugging_ON();
#endif

    AssertIntEQ(wolfSSH_Init(), WS_SUCCESS);

tests/auth.c:207

• In the NO_FILESYSTEM path, load_key(isEcc=1) always loads the nistp256 host key buffer (ecc_key_der_256_ssh). This will break configurations where nistp256 is disabled but nistp384/nistp521 are enabled (or where the server prefers a different curve), even though certs_test.h provides 384/521 buffers and ECC_PATH already selects those files for filesystem builds. Update the NO_FILESYSTEM branch to select ecc_key_der_{256,384,521}_ssh based on the same curve macros (or fall back to RSA when the matching ECC key isn’t available).

    bufName = isEcc ? ECC_PATH : "./keys/server-key-rsa.der";
    sz = load_file(bufName, buf, &bufSz);
#else
    /* using buffers instead */
    if (isEcc) {

---

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