From 7d9d82717314fd4e87388c596104e5bf3f6bb50e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 10 Apr 2024 15:34:31 -0700 Subject: [PATCH] Changing Channels 1. Remove the now unused states from the accept state list. Update the state machine to finish on ACCEPT_DONE. 2. Replace the exFds set with writeFds. Call worker if writeFds also set for the ssh socket. 3. If the worker returns WS_WANT_WRITE, WS_WANT_READ, or WS_REKEYING, go ahead and call select again then worker. 4. Handle doing SCP or SFTP later after handling the socket. 5. Add some missing "\n" on error logging in the echoserver. 6. In the sftp test script, clean recommendations from shellcheck. Removed the "-e" options from the echos while removing the newlines; echos with newlines get an additional echo. Removed redundant calls to the pwd program when PWD is already set. --- examples/echoserver/echoserver.c | 46 ++++++++++++---------- scripts/sftp.test | 66 ++++++++++++++++++-------------- src/ssh.c | 5 +-- src/wolfsftp.c | 16 -------- wolfssh/internal.h | 12 +----- 5 files changed, 65 insertions(+), 80 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 5574d5d71..76a0a633d 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -832,13 +832,14 @@ static int ssh_worker(thread_ctx_t* threadCtx) ChildRunning = 1; while (ChildRunning) { - fd_set readFds, exFds; + fd_set readFds, writeFds; WS_SOCKET_T maxFd; int cnt_r, cnt_w = 0; FD_ZERO(&readFds); - FD_ZERO(&exFds); + FD_ZERO(&writeFds); FD_SET(sshFd, &readFds); + FD_SET(sshFd, &writeFds); maxFd = sshFd; if (threadCtx->shellCtx.isConnected) { @@ -872,17 +873,16 @@ static int ssh_worker(thread_ctx_t* threadCtx) || threadCtx->fwdCbCtx.state == FWD_STATE_DIRECT_CONNECTED)) { FD_SET(fwdFd, &readFds); - FD_SET(fwdFd, &exFds); if (fwdFd > maxFd) maxFd = fwdFd; } #endif /* WOLFSSH_FWD */ - rc = select((int)maxFd + 1, &readFds, NULL, &exFds, NULL); + rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, NULL); if (rc == -1) break; - if (FD_ISSET(sshFd, &readFds)) { + if (FD_ISSET(sshFd, &readFds) || FD_ISSET(sshFd, &writeFds)) { word32 lastChannel = 0; /* The following tries to read from the first channel inside @@ -892,16 +892,6 @@ static int ssh_worker(thread_ctx_t* threadCtx) channel. The additional channel is only used with the agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); - #ifdef WOLFSSH_SCP - if (threadCtx->doScp) { - return WS_SCP_INIT; - } - #endif - #ifdef WOLFSSH_SFTP - if (threadCtx->doSftp) { - return WS_SFTP_COMPLETE; - } - #endif if (cnt_r < 0) { rc = wolfSSH_get_error(ssh); if (rc == WS_CHAN_RXD) { @@ -1011,14 +1001,28 @@ static int ssh_worker(thread_ctx_t* threadCtx) #endif continue; } - else if (rc != WS_WANT_READ && rc != WS_REKEYING) { + else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE + && rc != WS_REKEYING) { #ifdef SHELL_DEBUG printf("Break:read sshFd returns %d: errno =%x\n", cnt_r, errno); #endif break; } + else { + continue; + } + } + #ifdef WOLFSSH_SCP + if (threadCtx->doScp) { + return WS_SCP_INIT; } + #endif + #ifdef WOLFSSH_SFTP + if (threadCtx->doSftp) { + return WS_SFTP_COMPLETE; + } + #endif } #ifdef WOLFSSH_SHELL if (threadCtx->shellCtx.isConnected && !threadCtx->echo) { @@ -2390,13 +2394,13 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) case 'p': if (myoptarg == NULL) { - ES_ERROR("NULL port value"); + ES_ERROR("NULL port value\n"); } else { port = (word16)atoi(myoptarg); #if !defined(NO_MAIN_DRIVER) || defined(USE_WINDOWS_API) if (port == 0) { - ES_ERROR("port number cannot be 0"); + ES_ERROR("port number cannot be 0\n"); } #endif } @@ -2448,7 +2452,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) #ifdef WOLFSSH_TEST_BLOCK if (!nonBlock) { - ES_ERROR("Use -N when testing forced non blocking"); + ES_ERROR("Use -N when testing forced non blocking\n"); } #endif @@ -2535,7 +2539,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) keyLoadBuf = (byte*)WMALLOC(EXAMPLE_KEYLOAD_BUFFER_SZ, NULL, 0); if (keyLoadBuf == NULL) { - ES_ERROR("Error allocating keyLoadBuf"); + ES_ERROR("Error allocating keyLoadBuf\n"); } #else keyLoadBuf = buf; @@ -2761,7 +2765,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) &clientAddrSz); #endif if (clientFd == -1) { - ES_ERROR("tcp accept failed"); + ES_ERROR("tcp accept failed\n"); } if (nonBlock) diff --git a/scripts/sftp.test b/scripts/sftp.test index 69927e977..7afb12d6e 100755 --- a/scripts/sftp.test +++ b/scripts/sftp.test @@ -3,16 +3,17 @@ # sftp local test no_pid=-1 -server_pid=$no_pid -ready_file=`pwd`/wolfssh_sftp_ready$$ +server_pid="$no_pid" +ready_file="$PWD/wolfssh_sftp_ready$$" counter=0 nonblockingOnly=0 -[ ! -x ./examples/sftpclient/wolfsftp ] && echo -e "\n\nwolfSFTP client doesn't exist" && exit 1 +[ ! -x ./examples/sftpclient/wolfsftp ] && echo && echo "wolfSFTP client doesn't exist" && exit 1 # test for if the SFTP client works -./examples/sftpclient/wolfsftp -h | grep NO_WOLFSSH_CLIENT -if [ $? -eq 0 ] +./examples/sftpclient/wolfsftp -h | grep -q NO_WOLFSSH_CLIENT +RESULT=$? +if [ $RESULT -eq 0 ] then echo "macro NO_WOLFSSH_CLIENT was used" echo "skipping test" @@ -20,10 +21,11 @@ then fi # test for nonblocking only -./examples/client/client -h | grep WOLFSSH_TEST_BLOCK -if [ $? -eq 0 ] +./examples/client/client -h | grep -q WOLFSSH_TEST_BLOCK +RESULT=$? +if [ $RESULT -eq 0 ] then - echo "macro NO_WOLFSSH_CLIENT was used" + echo "macro WOLFSSH_TEST_BLOCK was used" nonblockingOnly=1 fi @@ -31,27 +33,27 @@ fi create_port() { while [ ! -s "$ready_file" ] && [ "$counter" -lt 20 ]; do - echo -e "waiting for ready file..." + echo "waiting for ready file..." sleep 0.1 counter=$((counter+ 1)) done - if test -e $ready_file; then - echo -e "found ready file, starting client..." + if test -e "$ready_file"; then + echo "found ready file, starting client..." # get created port 0 ephemeral port - port=`cat $ready_file` + port=$(cat "$ready_file") else - echo -e "NO ready file ending test..." + echo "NO ready file ending test..." do_cleanup exit 1 fi } remove_ready_file() { - if test -e $ready_file; then - echo -e "removing existing ready file" - rm $ready_file + if test -e "$ready_file"; then + echo "removing existing ready file" + rm "$ready_file" fi } @@ -69,23 +71,25 @@ do_cleanup() { do_trap() { echo "got trap" do_cleanup - exit -1 + exit 1 } trap do_trap INT TERM -[ ! -x ./examples/sftpclient/wolfsftp ] && echo -e "\n\nClient doesn't exist" && exit 1 +[ ! -x ./examples/sftpclient/wolfsftp ] && echo && echo "Client doesn't exist" && exit 1 if [ $nonblockingOnly = 0 ]; then echo "Test basic connection" - ./examples/echoserver/echoserver -1 -R $ready_file & + ./examples/echoserver/echoserver -1 -R "$ready_file" & server_pid=$! + counter=0 create_port - echo "exit" | ./examples/sftpclient/wolfsftp -u jill -P upthehill -p $port + echo "exit" | ./examples/sftpclient/wolfsftp -u jill -P upthehill -p "$port" RESULT=$? remove_ready_file if [ $RESULT -ne 0 ]; then - echo -e "\n\nfailed to connect" + echo + echo "failed to connect" do_cleanup exit 1 fi @@ -93,14 +97,16 @@ fi # Test non blocking connection echo "Test non blocking connection" -./examples/echoserver/echoserver -N -1 -R $ready_file & +./examples/echoserver/echoserver -N -1 -R "$ready_file" & server_pid=$! +counter=0 create_port -echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p $port +echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p "$port" RESULT=$? remove_ready_file if [ $RESULT -ne 0 ]; then - echo -e "\n\nfailed to connect" + echo + echo "failed to connect" do_cleanup exit 1 fi @@ -108,21 +114,23 @@ fi # Test of setting directory if [ $nonblockingOnly = 0 ]; then echo "Test of setting directory" - PWD=`pwd` - ./examples/echoserver/echoserver -d $PWD/examples -1 -R $ready_file & + ./examples/echoserver/echoserver -d "$PWD"/examples -1 -R "$ready_file" & server_pid=$! + counter=0 create_port - echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p $port + echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p "$port" RESULT=$? remove_ready_file if [ $RESULT -ne 0 ]; then - echo -e "\n\nfailed to connect" + echo + echo "failed to connect" do_cleanup exit 1 fi fi -echo -e "\nALL Tests Passed" +echo +echo "ALL Tests Passed" exit 0 diff --git a/src/ssh.c b/src/ssh.c index fbfd49504..32bb6b2a0 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -422,7 +422,7 @@ int wolfSSH_accept(WOLFSSH* ssh) /* check if data pending to be sent */ if (ssh->outputBuffer.length > 0 && - ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) { + ssh->acceptState < ACCEPT_DONE) { if ((ssh->error = wolfSSH_SendPacket(ssh)) == WS_SUCCESS) { WLOG(WS_LOG_DEBUG, "Sent pending packet"); @@ -430,8 +430,7 @@ int wolfSSH_accept(WOLFSSH* ssh) if (ssh->acceptState != ACCEPT_SERVER_VERSION_SENT && ssh->acceptState != ACCEPT_SERVER_USERAUTH_ACCEPT_SENT && ssh->acceptState != ACCEPT_SERVER_KEXINIT_SENT && - ssh->acceptState != ACCEPT_KEYED && - ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT) { + ssh->acceptState != ACCEPT_KEYED) { WLOG(WS_LOG_DEBUG, "Advancing accept state"); ssh->acceptState++; } diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 0a90f10f6..493efa7a1 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1126,22 +1126,6 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh) if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE) ssh->error = WS_SUCCESS; - /* check accept is done, if not call wolfSSH accept */ - if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) { - byte name[] = "sftp"; - - WLOG(WS_LOG_SFTP, "Trying to do SSH accept first"); - if ((ret = wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, - name, sizeof(name) - 1)) != WS_SUCCESS) { - WLOG(WS_LOG_SFTP, "Unable to set subsystem channel type"); - return ret; - } - - if ((ret = wolfSSH_accept(ssh)) != WS_SUCCESS) { - return ret; - } - } - switch (ssh->sftpState) { case SFTP_BEGIN: case SFTP_EXT: diff --git a/wolfssh/internal.h b/wolfssh/internal.h index a0fe8bab3..2b30a734d 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1012,17 +1012,7 @@ enum AcceptStates { ACCEPT_SERVER_USERAUTH_ACCEPT_SENT, ACCEPT_CLIENT_USERAUTH_DONE, ACCEPT_SERVER_USERAUTH_SENT, - ACCEPT_SERVER_CHANNEL_ACCEPT_SENT, - ACCEPT_CLIENT_SESSION_ESTABLISHED, -#ifdef WOLFSSH_SCP - ACCEPT_INIT_SCP_TRANSFER, -#endif -#ifdef WOLFSSH_SFTP - ACCEPT_INIT_SFTP, -#endif -#ifdef WOLFSSH_AGENT - ACCEPT_INIT_AGENT, -#endif + ACCEPT_DONE };