tests: fix wolfSSHd harness bugs that hid skipped and failed tests

.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

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
 
 

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,16 +201,21 @@ 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 \
         -h "$TEST_HOST" -p "$TEST_PORT" ) > /dev/null 2>&1
     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,15 +465,24 @@ 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,
 # but background test daemons survive across CI steps that share this runner,
 # 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"

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
 

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
 

apps/wolfsshd/test/sshd_forcedcmd_test.sh

@@ -8,11 +8,21 @@ 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: 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 <<EOF > sshd_config_test_forcedcmd
 Port $TEST_PORT
 Protocol 2
@@ -22,8 +32,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,7 +59,7 @@ 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
 
 # A configured ForceCommand that is not "internal-sftp" must still permit the
@@ -64,8 +74,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,7 +86,7 @@ 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
 exit 0
 

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
 

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 <root>/../../../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,16 +79,17 @@ 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
 # "sudo env"). "UsePrivilegeSeparation no" is the worst case: old fallback = noop.
 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

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
 

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