From 9f7f07c15ac9f55119b6814865b92094f5440e2f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 4 Aug 2026 23:51:27 -0700 Subject: [PATCH 1/4] tests: fix StrictModes count aborting sshd suite grep -c prints 0 and exits 1 when nothing matches, so the "|| echo 0" fallback fired too and the count became "0\n0". The arithmetic error unwound bash out of the test block, skipping the last eleven tests while the summary still printed a pass and exited 0. - Use the bare grep -c result and default only the empty case. - Add a RUN_COMPLETE sentinel at the end of each branch that runs tests. - Check the sentinel before the summary so an abort exits non-zero. - Kill lingering daemons by process name, so the teardown does not kill the run itself before that check when invoked by a path holding "wolfsshd". --- apps/wolfsshd/test/run_all_sshd_tests.sh | 27 +++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index f1ee3e334..65cc89aa9 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -63,6 +63,12 @@ done TOTAL=0 SKIPPED=0 +# Set as the last statement of each branch that runs tests, and checked before +# the summary. A shell expansion error (a bad arithmetic expansion, say) unwinds +# bash out of the whole enclosing compound command, skipping every remaining +# test but still running the trailing summary -- which then reports a pass. This +# flag makes that abort exit non-zero instead of going green. +RUN_COMPLETE=0 # validate the requested test before any setup so a bad name does not leave # a wolfSSHd running @@ -195,8 +201,12 @@ run_strictmodes_authkeys_negative_test() { # AND make the daemon log the StrictModes rejection, so the failure is for # the right reason and not an unrelated client error. Count existing # rejection lines first so a re-run is not confused by stale matches. + # Do not add "|| echo 0" here: grep -c prints 0 AND exits 1 when there is no + # match, so the fallback appends a second 0 and every later use of the count + # is a syntax error. Default the empty (missing/unreadable log) case instead. local before - before=$(grep -c "failed StrictModes check" log.txt 2>/dev/null || echo 0) + before=$(grep -c "failed StrictModes check" log.txt 2>/dev/null) + before=${before:-0} chmod 0666 authorized_keys_test ( cd ../../.. && $tmo ./examples/client/client -c 'exit' -u "$USER" \ -i ./keys/hansel-key-ecc.der -j ./keys/hansel-key-ecc.pub \ @@ -204,7 +214,8 @@ run_strictmodes_authkeys_negative_test() { local result=$? chmod 0644 authorized_keys_test local after - after=$(grep -c "failed StrictModes check" log.txt 2>/dev/null || echo 0) + after=$(grep -c "failed StrictModes check" log.txt 2>/dev/null) + after=${after:-0} if [ "$result" != 0 ] && [ "$after" -gt "$before" ]; then printf "PASSED\n" else @@ -357,6 +368,7 @@ if [[ -n "$MATCH" ]]; then printf "Shutting down test wolfSSHd\n" stop_wolfsshd fi + RUN_COMPLETE=1 else echo "Running all tests..." for test in "${test_cases[@]}"; do @@ -453,6 +465,7 @@ else if [ "$USING_LOCAL_HOST" == 1 ]; then run_test "sshd_ossh_cert_test.sh" fi + RUN_COMPLETE=1 fi # Teardown safety net: the start/stop pairs above stop each daemon they start, @@ -460,8 +473,16 @@ fi # and a later step (the valgrind "memory after close down" check) binds the same # port 22222. Make sure no test daemon lingers when this script exits so that # step does not fail with "tcp bind failed". Harmless when nothing is running. +# Match the process name, not the whole command line: "-f wolfsshd" also matches +# this script when it is invoked by a path holding "wolfsshd", killing the run +# before the check below and losing the summary. if [ "$USING_LOCAL_HOST" == 1 ]; then - sudo pkill -f "wolfsshd" 2>/dev/null || true + sudo pkill -x wolfsshd 2>/dev/null || true +fi + +if [ "$RUN_COMPLETE" != 1 ]; then + printf "ERROR: test run aborted before all tests ran\n" + exit 1 fi printf "All tests ran, $TOTAL passed, $SKIPPED skipped\n" From 07377424becdca249d26a5f9681b490748399825 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 5 Aug 2026 09:44:26 -0700 Subject: [PATCH 2/4] tests: save sshd test scripts' dir in TESTDIR Eight scripts saved their starting directory in PWD, which the shell rewrites on every cd, so the "cd $PWD" restore landed in the repository root. sshd_forcedcmd_test.sh's second scenario and the log count in sshd_x509_upn_fail.sh silently never did what they cover. - Save the starting directory in TESTDIR, as sshd_pubkey_reject_test.sh does. - Quote "$TESTDIR" at every cd, now that the saved value is really used. - Stop the daemon from a trap in sshd_forcedcmd_test.sh, so its now reachable second scenario cannot leave one on the shared port when set -e aborts. - Take start_wolfsshd's before and after daemon PID snapshots from pgrep -x instead of scraping every digit run out of "ps -e", which mixed the TIME field's clock digits in with the PID and, with a leftover daemon running, stopped the wrong process. - Let both snapshot pipelines fail, so a set -e caller survives no daemon being up and a daemon that dies after sudo returns is reported by the caller's own empty-PID check. --- apps/wolfsshd/test/error_return.sh | 8 +++--- apps/wolfsshd/test/sshd_bad_sftp_test.sh | 6 +++-- apps/wolfsshd/test/sshd_exec_test.sh | 6 +++-- apps/wolfsshd/test/sshd_forcedcmd_test.sh | 31 +++++++++++++++++----- apps/wolfsshd/test/sshd_large_sftp_test.sh | 6 +++-- apps/wolfsshd/test/sshd_scp_fail.sh | 6 +++-- apps/wolfsshd/test/sshd_x509_test.sh | 8 +++--- apps/wolfsshd/test/sshd_x509_upn_fail.sh | 7 +++-- apps/wolfsshd/test/start_sshd.sh | 16 ++++++++--- 9 files changed, 67 insertions(+), 27 deletions(-) diff --git a/apps/wolfsshd/test/error_return.sh b/apps/wolfsshd/test/error_return.sh index 8c876324b..9240818bb 100755 --- a/apps/wolfsshd/test/error_return.sh +++ b/apps/wolfsshd/test/error_return.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` cd ../../.. TEST_CLIENT="./examples/client/client" @@ -21,11 +23,11 @@ $TEST_CLIENT -c 'bash -c "(exit 2)"' -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h RESULT=$? if [ "$RESULT" != 2 ]; then echo "Expecting error return value of 2 for failed ls command, found $RESULT" - cd $PWD + cd "$TESTDIR" exit 1 fi -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_bad_sftp_test.sh b/apps/wolfsshd/test/sshd_bad_sftp_test.sh index 362de3bfe..c885b7011 100755 --- a/apps/wolfsshd/test/sshd_bad_sftp_test.sh +++ b/apps/wolfsshd/test/sshd_bad_sftp_test.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` . ./wolfssh_options.sh cd ../../.. @@ -35,6 +37,6 @@ if [ "$RESULT" = "0" ]; then fi rm -rf test-$$ -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_exec_test.sh b/apps/wolfsshd/test/sshd_exec_test.sh index 889bc24a2..6cdf20cd3 100755 --- a/apps/wolfsshd/test/sshd_exec_test.sh +++ b/apps/wolfsshd/test/sshd_exec_test.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` cd ../../.. TEST_CLIENT="./examples/client/client" @@ -22,6 +24,6 @@ $TEST_CLIENT -c 'ls' -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h "$1" -p "$2" set +e -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_forcedcmd_test.sh b/apps/wolfsshd/test/sshd_forcedcmd_test.sh index 9aa3adacc..881bd091e 100755 --- a/apps/wolfsshd/test/sshd_forcedcmd_test.sh +++ b/apps/wolfsshd/test/sshd_forcedcmd_test.sh @@ -8,11 +8,26 @@ if [ -z "$1" ] || [ -z "$2" ]; then exit 1 fi -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved copy +# would not survive the cd to the repository root below. +TESTDIR=`pwd` USER=`whoami` TEST_PORT="$2" TEST_HOST="$1" source ./start_sshd.sh + +# Stop the daemon on every exit path. From the "set -e" below onward an aborted +# client run would otherwise leave a root daemon holding the shared test port, +# and every later test in the suite would talk to this config. +cleanup() { + if [ -n "$PID" ]; then + stop_wolfsshd + PID="" + fi + return 0 +} +trap cleanup EXIT + cat < sshd_config_test_forcedcmd Port $TEST_PORT Protocol 2 @@ -22,8 +37,8 @@ PasswordAuthentication yes PermitEmptyPasswords no UsePrivilegeSeparation no UseDNS no -HostKey $PWD/../../../keys/server-key.pem -AuthorizedKeysFile $PWD/authorized_keys_test +HostKey $TESTDIR/../../../keys/server-key.pem +AuthorizedKeysFile $TESTDIR/authorized_keys_test Match User $USER ForceCommand internal-sftp @@ -49,8 +64,9 @@ fi set -e echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT -cd $PWD +cd "$TESTDIR" stop_wolfsshd +PID="" # A configured ForceCommand that is not "internal-sftp" must still permit the # SFTP subsystem. Only a certificate force-command denies file transfer, so a @@ -64,8 +80,8 @@ PasswordAuthentication yes PermitEmptyPasswords no UsePrivilegeSeparation no UseDNS no -HostKey $PWD/../../../keys/server-key.pem -AuthorizedKeysFile $PWD/authorized_keys_test +HostKey $TESTDIR/../../../keys/server-key.pem +AuthorizedKeysFile $TESTDIR/authorized_keys_test Match User $USER ForceCommand /bin/echo @@ -76,8 +92,9 @@ cd ../../.. echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT -cd $PWD +cd "$TESTDIR" stop_wolfsshd +PID="" exit 0 diff --git a/apps/wolfsshd/test/sshd_large_sftp_test.sh b/apps/wolfsshd/test/sshd_large_sftp_test.sh index 6699eae28..4f177a375 100755 --- a/apps/wolfsshd/test/sshd_large_sftp_test.sh +++ b/apps/wolfsshd/test/sshd_large_sftp_test.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` . ./wolfssh_options.sh cd ../../.. @@ -56,6 +58,6 @@ rm -f "$REMOTE_FILE" set +e -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_scp_fail.sh b/apps/wolfsshd/test/sshd_scp_fail.sh index 02569853a..04382faa6 100755 --- a/apps/wolfsshd/test/sshd_scp_fail.sh +++ b/apps/wolfsshd/test/sshd_scp_fail.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` . ./wolfssh_options.sh cd ../../.. @@ -48,6 +50,6 @@ fi rm -rf test-$$ rm testout.dat -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_x509_test.sh b/apps/wolfsshd/test/sshd_x509_test.sh index d52b03c24..e497a707e 100755 --- a/apps/wolfsshd/test/sshd_x509_test.sh +++ b/apps/wolfsshd/test/sshd_x509_test.sh @@ -2,7 +2,9 @@ # sshd local test -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved +# copy would not survive the cd to the repository root below. +TESTDIR=`pwd` cd ../../.. if [ -z "$1" ] || [ -z "$2" ] || [ -z "$3" ]; then @@ -28,11 +30,11 @@ $TEST_CLIENT -X -c 'ls error' -u $3 -i "$PRIVATE_KEY" -J "$PUBLIC_KEY" -A "$CA_C # check stderr output was caught if [ ! -s error.txt ]; then echo "No stderr data was found when expected!!" - cd $PWD + cd "$TESTDIR" exit 1 fi rm -f error.txt -cd $PWD +cd "$TESTDIR" exit 0 diff --git a/apps/wolfsshd/test/sshd_x509_upn_fail.sh b/apps/wolfsshd/test/sshd_x509_upn_fail.sh index 0a19dcdbe..8f0ca428d 100755 --- a/apps/wolfsshd/test/sshd_x509_upn_fail.sh +++ b/apps/wolfsshd/test/sshd_x509_upn_fail.sh @@ -5,7 +5,10 @@ # "other.example", while the client certificate carries the UPN realm # "example". The wolfSSHd UPN domain check must therefore reject the cert. -PWD=`pwd` +# Not named PWD: the shell rewrites that variable on every cd, so a saved copy +# would not survive the cd to the repository root below -- and the log counted +# after the client run lives here, not there. +TESTDIR=`pwd` . ./wolfssh_options.sh # The UPN domain check is compiled only when wolfSSL is built with FPKI. @@ -37,7 +40,7 @@ echo "$TEST_CLIENT -X -c 'pwd' -u $3 -i $PRIVATE_KEY -J $PUBLIC_KEY -A $CA_CERT $TEST_CLIENT -X -c 'pwd' -u "$3" -i "$PRIVATE_KEY" -J "$PUBLIC_KEY" -A "$CA_CERT" -h "$1" -p "$2" RESULT=$? -cd "$PWD" +cd "$TESTDIR" # Give the daemon child a moment to flush its rejection to the log. sleep 1 diff --git a/apps/wolfsshd/test/start_sshd.sh b/apps/wolfsshd/test/start_sshd.sh index 1c553c07a..3acb1da16 100755 --- a/apps/wolfsshd/test/start_sshd.sh +++ b/apps/wolfsshd/test/start_sshd.sh @@ -6,7 +6,13 @@ SSHD_KEYDIR="" # starts up a sshd session, takes in the sshd_config file as an argument start_wolfsshd() { - CURRENT_PIDS=`ps -e | grep wolfsshd | grep -oE "[0-9]+"` + # Snapshot the PIDs of any daemon already running so the new one can be + # picked out below. PIDs only: "ps -e" also prints TIME, and scraping + # every digit run off that line mixes clock digits in with the PID. + # Sorted so both snapshots order the same way. No daemon running is the + # normal case and pgrep exits 1 on no match, which would end a caller + # running under "set -e". + CURRENT_PIDS=`pgrep -x wolfsshd | sort -n` || true ORIGCFG="$1" CONFIG="$ORIGCFG" @@ -86,9 +92,11 @@ EOF SSHD_BIN="${SSHD_BIN:-../wolfsshd}" sudo env $SSHD_ENV "$SSHD_BIN" -d -E ./log.txt -f "$CONFIG" - # set the PID of started sshd - NEW_PID=`ps -e | grep wolfsshd | grep -oE "[0-9]+"` - PID=`diff <(echo "$CURRENT_PIDS") <(echo "$NEW_PID") | grep '>' | grep -oE "[0-9]+" | head -n1` + # The PID of the started sshd is the one present now that was not there + # before. The daemon can still die after sudo returns, so guard the same + # way and let the caller's empty-PID check report it. + NEW_PIDS=`pgrep -x wolfsshd | sort -n` || true + PID=`diff <(echo "$CURRENT_PIDS") <(echo "$NEW_PIDS") | sed -n 's/^> *//p' | head -n1` printf "SSHD running on PID $PID\n" } From 15470845fd8b7cb78c8a792f63ca8f2ca5cf312c Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 5 Aug 2026 10:08:46 -0700 Subject: [PATCH 3/4] tests: fix privdrop test's client key paths sshd_privdrop_fail_test.sh runs from apps/wolfsshd/test and handed the example clients relative key paths, but the clients call ChangeToWolfSshRoot() before parsing arguments. Every client died at "Error setting private key" and the test blamed the privilege drop. - Anchor the key, payload and client paths at the script's own directory. - Rename the saved directory to TESTDIR so a later cd cannot clobber it. - Report "no fork at all" separately in the timeout diagnostic. - Print the client's own output at every failure exit, so a client that never connects cannot be read as a daemon fault. - Keep the client logs like log.txt, gitignored and removed on success. --- .gitignore | 1 + apps/wolfsshd/test/sshd_privdrop_fail_test.sh | 59 ++++++++++++++----- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/.gitignore b/.gitignore index 4c00c26ff..23916c9dc 100644 --- a/.gitignore +++ b/.gitignore @@ -70,6 +70,7 @@ apps/wolfssh/wolfssh apps/wolfsshd/wolfsshd apps/wolfsshd/test/test_configuration apps/wolfsshd/test/sshd_privdrop_preload.so +apps/wolfsshd/test/privdrop_client_*.log apps/wolfsshd/test/log.txt apps/wolfsshd/test/sshd_config_* apps/wolfsshd/test/authorized_keys_test diff --git a/apps/wolfsshd/test/sshd_privdrop_fail_test.sh b/apps/wolfsshd/test/sshd_privdrop_fail_test.sh index 73a22c0d4..67623a075 100755 --- a/apps/wolfsshd/test/sshd_privdrop_fail_test.sh +++ b/apps/wolfsshd/test/sshd_privdrop_fail_test.sh @@ -12,7 +12,9 @@ if [ -z "$1" ] || [ -z "$2" ]; then exit 1 fi -PWD=`pwd` +# Not PWD: that is bash's own variable and the shell rewrites it on every cd, +# so a saved copy is gone by the time it is read. +TESTDIR=`pwd` USER=`whoami` TEST_HOST="$1" @@ -46,15 +48,24 @@ if [ -f ./log.txt ]; then fi touch log.txt -TEST_CLIENT="../../../examples/client/client" -SFTP_CLIENT="../../../examples/sftpclient/wolfsftp" -SCP_CLIENT="../../../examples/scpclient/wolfscp" -PRIVATE_KEY="../../../keys/hansel-key-ecc.der" -PUBLIC_KEY="../../../keys/hansel-key-ecc.pub" +ROOT="$TESTDIR/../../.." +TEST_CLIENT="$ROOT/examples/client/client" +SFTP_CLIENT="$ROOT/examples/sftpclient/wolfsftp" +SCP_CLIENT="$ROOT/examples/scpclient/wolfscp" + +# Absolute, and that matters. Unlike the other sshd tests this one runs from +# its own directory rather than the repository root, and the example clients +# call ChangeToWolfSshRoot() before parsing anything, so every path they are +# handed is resolved from the repository root instead of here. Relative paths +# were read as /../../../keys/... and the clients died at "Error setting +# private key" without ever opening a socket. +PRIVATE_KEY="$ROOT/keys/hansel-key-ecc.der" +PUBLIC_KEY="$ROOT/keys/hansel-key-ecc.pub" # Small payload for the sftp/scp transfers. The connection dies at the failed -# drop long before any data moves, so the contents do not matter. -PAYLOAD="privdrop_payload.txt" +# drop long before any data moves, so the contents do not matter. Absolute for +# the same reason: the sftp client reads it after the chdir above. +PAYLOAD="$TESTDIR/privdrop_payload.txt" echo "privdrop" > "$PAYLOAD" source ./start_sshd.sh @@ -68,8 +79,8 @@ PasswordAuthentication yes PermitEmptyPasswords no UsePrivilegeSeparation no UseDNS no -HostKey $PWD/../../../keys/server-key.pem -AuthorizedKeysFile $PWD/authorized_keys_test +HostKey $ROOT/keys/server-key.pem +AuthorizedKeysFile $TESTDIR/authorized_keys_test EOF # Preload and arm the interposer via SSHD_ENV (start_sshd.sh passes it through @@ -77,7 +88,8 @@ EOF SSHD_ENV="LD_PRELOAD=$PRELOAD_LIB WOLFSSHD_FAULT_PRIVDROP=1" export SSHD_BIN SSHD_ENV -# Teardown on every exit path; log.txt is kept for debugging like the other tests. +# Teardown on every exit path; log.txt and the client logs are kept for +# debugging like the other tests, and removed on the success path below. cleanup() { stop_wolfsshd rm -f sshd_config_test_privdrop "$PAYLOAD" "$PRELOAD_LIB" @@ -94,6 +106,12 @@ fi DEADLINE=30 +# Every failure below is ambiguous without the client's own output. +client_said() { + echo " client said: `tail -n 3 "$CLIENT_LOG" | tr '\n' '|'`" + echo " full client output in $CLIENT_LOG" +} + # Drives one client; the connection dies, so its exit status is not checked. # Counts are per-call deltas since all three subsystems share the one log. check_subsystem() { @@ -104,7 +122,11 @@ check_subsystem() { BEFORE_CLOSE=`grep -c "Attempting to close down connection" log.txt` BEFORE_SPAWN=`grep -c "Spawned new process" log.txt` - "$@" > /dev/null 2>&1 & + # Keep the client's output. Its exit status is meaningless here (the + # connection is killed under it), but if it dies before opening a socket + # the daemon-side counters below stay flat and look like a daemon fault. + CLIENT_LOG="$TESTDIR/privdrop_client_$LABEL.log" + "$@" > "$CLIENT_LOG" 2>&1 & CLIENT_PID=$! # Wait for the connection child to fork and hit the failed drop, then take @@ -128,7 +150,12 @@ check_subsystem() { wait $CLIENT_PID > /dev/null 2>&1 if [ -z "$CHILD" ]; then - echo "FAIL: $LABEL never reached the privilege drop" + if [ "$AFTER_SPAWN" -eq "$BEFORE_SPAWN" ]; then + echo "FAIL: $LABEL daemon never forked a connection process" + else + echo "FAIL: $LABEL never reached the privilege drop" + fi + client_said exit 1 fi @@ -141,6 +168,7 @@ check_subsystem() { done if ps -p "$CHILD" > /dev/null 2>&1; then echo "FAIL: $LABEL connection process still running after ${DEADLINE}s" + client_said exit 1 fi @@ -149,6 +177,7 @@ check_subsystem() { AFTER_CLOSE=`grep -c "Attempting to close down connection" log.txt` if [ "$AFTER_CLOSE" -gt "$BEFORE_CLOSE" ]; then echo "FAIL: $LABEL handler continued after a failed privilege drop" + client_said exit 1 fi @@ -170,7 +199,9 @@ check_subsystem "sftp" \ # SCP_Subsystem. check_subsystem "scp" \ "$SCP_CLIENT" -u "$USER" -i "$PRIVATE_KEY" -j "$PUBLIC_KEY" \ - -S"$PWD/$PAYLOAD:." -H "$TEST_HOST" -p "$TEST_PORT" + -S"$PAYLOAD:." -H "$TEST_HOST" -p "$TEST_PORT" + +rm -f "$TESTDIR"/privdrop_client_*.log echo "PASS: all subsystems terminate on privilege-drop failure" exit 0 From 679d120ee3509f84e5d4c5ebd185bc9f43d34b4e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 19 Aug 2026 17:16:03 -0700 Subject: [PATCH 4/4] tests: guard stop_wolfsshd so it cannot fail its caller stop_wolfsshd killed $PID unconditionally. With the daemon already gone the kill failed, and under "set -e" that aborted the caller -- in sshd_forcedcmd_test.sh before PID was cleared, so the ForceCommand-SFTP scenario was silently skipped, the EXIT trap killed the dead pid a second time, and the script exited 1. - Guard on a non-empty PID, ignore a failed kill and return 0, so the function is safe to call from an EXIT trap. - Clear PID after stopping, so a second call cannot kill a recycled pid. - Remove the temp key dir even when no daemon was recorded, so a daemon that failed to start does not leak it. - Collapse sshd_forcedcmd_test.sh's cleanup() wrapper to a bare trap stop_wolfsshd EXIT now that the function guards itself. - Check the cd back to the test directory in sshd_x509_upn_fail.sh; the log it counts after the client run is the one there. --- apps/wolfsshd/test/sshd_forcedcmd_test.sh | 19 +++++--------- apps/wolfsshd/test/sshd_x509_upn_fail.sh | 4 ++- apps/wolfsshd/test/start_sshd.sh | 31 +++++++++++++++-------- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/apps/wolfsshd/test/sshd_forcedcmd_test.sh b/apps/wolfsshd/test/sshd_forcedcmd_test.sh index 881bd091e..024a54c40 100755 --- a/apps/wolfsshd/test/sshd_forcedcmd_test.sh +++ b/apps/wolfsshd/test/sshd_forcedcmd_test.sh @@ -16,17 +16,12 @@ TEST_PORT="$2" TEST_HOST="$1" source ./start_sshd.sh -# Stop the daemon on every exit path. From the "set -e" below onward an aborted -# client run would otherwise leave a root daemon holding the shared test port, -# and every later test in the suite would talk to this config. -cleanup() { - if [ -n "$PID" ]; then - stop_wolfsshd - PID="" - fi - return 0 -} -trap cleanup EXIT +# Stop the daemon on every exit path: the shell-login check below exits +# non-zero, and from the "set -e" onward an aborted client run would leave a +# root daemon holding the shared test port, so every later test in the suite +# would talk to this config. stop_wolfsshd clears PID, so this is a no-op after +# each explicit stop below. +trap stop_wolfsshd EXIT cat < sshd_config_test_forcedcmd Port $TEST_PORT @@ -66,7 +61,6 @@ echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p cd "$TESTDIR" stop_wolfsshd -PID="" # A configured ForceCommand that is not "internal-sftp" must still permit the # SFTP subsystem. Only a certificate force-command denies file transfer, so a @@ -94,7 +88,6 @@ echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p cd "$TESTDIR" stop_wolfsshd -PID="" exit 0 diff --git a/apps/wolfsshd/test/sshd_x509_upn_fail.sh b/apps/wolfsshd/test/sshd_x509_upn_fail.sh index 8f0ca428d..64dfd8bee 100755 --- a/apps/wolfsshd/test/sshd_x509_upn_fail.sh +++ b/apps/wolfsshd/test/sshd_x509_upn_fail.sh @@ -40,7 +40,9 @@ echo "$TEST_CLIENT -X -c 'pwd' -u $3 -i $PRIVATE_KEY -J $PUBLIC_KEY -A $CA_CERT $TEST_CLIENT -X -c 'pwd' -u "$3" -i "$PRIVATE_KEY" -J "$PUBLIC_KEY" -A "$CA_CERT" -h "$1" -p "$2" RESULT=$? -cd "$TESTDIR" +# Back to the test dir: the log counted below is the one here, so a failed cd +# would silently count matches in the repository root's log.txt instead. +cd "$TESTDIR" || exit 1 # Give the daemon child a moment to flush its rejection to the log. sleep 1 diff --git a/apps/wolfsshd/test/start_sshd.sh b/apps/wolfsshd/test/start_sshd.sh index 3acb1da16..f46df3b7a 100755 --- a/apps/wolfsshd/test/start_sshd.sh +++ b/apps/wolfsshd/test/start_sshd.sh @@ -100,22 +100,33 @@ EOF printf "SSHD running on PID $PID\n" } -# closes down the sshd session taking argument $1 as the PID of the session +# closes down the sshd session started by start_wolfsshd, using $PID. +# Idempotent and safe to call from an EXIT trap: with no daemon recorded there +# is nothing to kill, and neither an already-exited daemon nor a missing temp +# dir may become the caller's exit status under "set -e". stop_wolfsshd() { - printf "Stopping SSHD, killing pid $PID\n" - sudo kill $PID + if [ -n "$PID" ]; then + printf "Stopping SSHD, killing pid $PID\n" + sudo kill $PID || true - # Wait for the process to actually exit so a subsequent start_wolfsshd on - # the same port doesn't race the listening socket's release (EADDRINUSE). - for i in $(seq 1 50); do - sudo kill -0 $PID 2>/dev/null || break - sleep 0.1 - done + # Wait for the process to actually exit so a subsequent start_wolfsshd on + # the same port doesn't race the listening socket's release (EADDRINUSE). + for i in $(seq 1 50); do + sudo kill -0 $PID 2>/dev/null || break + sleep 0.1 + done + + # Cleared so a second call -- an EXIT trap after an explicit stop -- is + # a no-op rather than a kill of whatever pid has since been recycled. + PID="" + fi # The temp dir is owned by the invoking user, so its root-owned key copies - # can be removed without sudo. + # can be removed without sudo. Done even when no daemon was recorded, so a + # daemon that failed to start does not leak it. if [ -n "$SSHD_KEYDIR" ]; then rm -rf "$SSHD_KEYDIR" SSHD_KEYDIR="" fi + return 0 }