From 6dade0cdfe70428efae7dbf72f4976e360bbd7be Mon Sep 17 00:00:00 2001 From: Lyor Goldstein Date: Tue, 19 Sep 2023 19:24:42 +0300 Subject: [PATCH] [GH-328] Added configurable timeout(s) to DefaultSftpClient --- CHANGES.md | 1 + .../test/java/org/apache/sshd/client/ClientTest.java | 4 ++-- .../org/apache/sshd/common/forward/Sshd1033Test.java | 5 +++-- .../org/apache/sshd/util/test/BaseTestSupport.java | 4 ++-- .../sshd/sftp/client/impl/AbstractSftpClient.java | 8 ++++++++ .../sshd/sftp/client/impl/DefaultSftpClient.java | 6 ++++-- .../apache/sshd/sftp/client/SftpPerformanceTest.java | 10 ++++++++-- 7 files changed, 28 insertions(+), 10 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index aae82ad34..9c3b51f95 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -26,6 +26,7 @@ ## Bug Fixes +* [GH-328](https://github.com/apache/mina-sshd/issues/328) Added configurable timeout(s) to `DefaultSftpClient`. * [GH-370](https://github.com/apache/mina-sshd/issues/370) Also compare file keys in `ModifiableFileWatcher`. * [GH-371](https://github.com/apache/mina-sshd/issues/371) Fix channel pool in `SftpFileSystem`. * [GH-383](https://github.com/apache/mina-sshd/issues/383) Use correct default `OpenOption`s in `SftpFileSystemProvider.newFileChannel()`. diff --git a/sshd-core/src/test/java/org/apache/sshd/client/ClientTest.java b/sshd-core/src/test/java/org/apache/sshd/client/ClientTest.java index 3266c67ec..a41c2e1fc 100644 --- a/sshd-core/src/test/java/org/apache/sshd/client/ClientTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/client/ClientTest.java @@ -655,7 +655,7 @@ public void operationComplete(OpenFuture future) { @Override public void operationComplete(IoReadFuture future) { try { - future.verify(); + future.verify(OPEN_TIMEOUT); Buffer buffer = future.getBuffer(); baosOut.write(buffer.array(), buffer.rpos(), buffer.available()); buffer.rpos(buffer.rpos() + buffer.available()); @@ -675,7 +675,7 @@ public void operationComplete(IoReadFuture future) { @Override public void operationComplete(IoReadFuture future) { try { - future.verify(); + future.verify(OPEN_TIMEOUT); Buffer buffer = future.getBuffer(); baosErr.write(buffer.array(), buffer.rpos(), buffer.available()); buffer.rpos(buffer.rpos() + buffer.available()); diff --git a/sshd-core/src/test/java/org/apache/sshd/common/forward/Sshd1033Test.java b/sshd-core/src/test/java/org/apache/sshd/common/forward/Sshd1033Test.java index 0160939ce..6f166fdf1 100644 --- a/sshd-core/src/test/java/org/apache/sshd/common/forward/Sshd1033Test.java +++ b/sshd-core/src/test/java/org/apache/sshd/common/forward/Sshd1033Test.java @@ -106,9 +106,10 @@ protected void doTest(boolean testLocal, boolean testDynamic) throws IOException client.setServerKeyVerifier(AcceptAllServerKeyVerifier.INSTANCE); client.start(); - try (ClientSession session = client.connect("temp", "localhost", sshPort).verify().getClientSession()) { + try (ClientSession session + = client.connect("temp", "localhost", sshPort).verify(CONNECT_TIMEOUT).getClientSession()) { session.addPasswordIdentity("temp"); - session.auth().verify(); + session.auth().verify(AUTH_TIMEOUT); if (testLocal) { LOGGER.info("================== Local =================="); diff --git a/sshd-core/src/test/java/org/apache/sshd/util/test/BaseTestSupport.java b/sshd-core/src/test/java/org/apache/sshd/util/test/BaseTestSupport.java index 01c2a9bb4..9400ddb0c 100644 --- a/sshd-core/src/test/java/org/apache/sshd/util/test/BaseTestSupport.java +++ b/sshd-core/src/test/java/org/apache/sshd/util/test/BaseTestSupport.java @@ -46,8 +46,8 @@ public abstract class BaseTestSupport extends JUnitTestSupport { public static final String TEST_LOCALHOST = System.getProperty("org.apache.sshd.test.localhost", SshdSocketAddress.LOCALHOST_IPV4); - public static final Duration CONNECT_TIMEOUT = CoreTestSupportUtils.getTimeout("connect", Duration.ofSeconds(7)); - public static final Duration AUTH_TIMEOUT = CoreTestSupportUtils.getTimeout("auth", Duration.ofSeconds(5)); + public static final Duration CONNECT_TIMEOUT = CoreTestSupportUtils.getTimeout("connect", Duration.ofSeconds(10)); + public static final Duration AUTH_TIMEOUT = CoreTestSupportUtils.getTimeout("auth", Duration.ofSeconds(8)); public static final Duration OPEN_TIMEOUT = CoreTestSupportUtils.getTimeout("open", Duration.ofSeconds(9)); public static final Duration DEFAULT_TIMEOUT = CoreTestSupportUtils.getTimeout("default", Duration.ofSeconds(5)); public static final Duration CLOSE_TIMEOUT = CoreTestSupportUtils.getTimeout("close", Duration.ofSeconds(15)); diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java index 1c7b24cf6..ecc90ec48 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java @@ -24,6 +24,7 @@ import java.nio.channels.FileChannel; import java.nio.charset.Charset; import java.nio.file.attribute.FileTime; +import java.time.Duration; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -36,6 +37,7 @@ import org.apache.sshd.client.channel.ClientChannel; import org.apache.sshd.client.subsystem.AbstractSubsystemClient; +import org.apache.sshd.common.Property; import org.apache.sshd.common.SshConstants; import org.apache.sshd.common.SshException; import org.apache.sshd.common.channel.Channel; @@ -61,6 +63,12 @@ public abstract class AbstractSftpClient extends AbstractSubsystemClient implements FullAccessSftpClient, SftpErrorDataHandler { public static final int INIT_COMMAND_SIZE = Byte.BYTES /* command */ + Integer.BYTES /* version */; + /** + * Property that can be used on the {@link org.apache.sshd.common.FactoryManager} to control the internal timeout + * used by the client to complete the buffer sending in {@link #send(int, Buffer)}. + */ + public static final Property SFTP_CLIENT_CMD_TIMEOUT + = Property.duration("sftp-client-cmd-timeout", Duration.ofSeconds(30L)); protected final SftpErrorDataHandler errorDataHandler; diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/DefaultSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/DefaultSftpClient.java index 25792a048..07372de88 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/DefaultSftpClient.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/DefaultSftpClient.java @@ -298,7 +298,8 @@ public int send(int cmd, Buffer buffer) throws IOException { ClientChannel clientChannel = getClientChannel(); IoOutputStream asyncIn = clientChannel.getAsyncIn(); IoWriteFuture writeFuture = asyncIn.writeBuffer(buf); - writeFuture.verify(); + Duration cmdTimeout = SFTP_CLIENT_CMD_TIMEOUT.getRequired(clientChannel); + writeFuture.verify(cmdTimeout); return id; } @@ -377,8 +378,9 @@ protected void init(ClientSession session, SftpVersionSelector initialVersionSel if (traceEnabled) { log.trace("init({}) send SSH_FXP_INIT - initial version={}", clientChannel, initialVersion); } + IoWriteFuture writeFuture = asyncIn.writeBuffer(buf); - writeFuture.verify(); + writeFuture.verify(initializationTimeout); if (traceEnabled) { log.trace("init({}) wait for SSH_FXP_INIT respose (timeout={})", clientChannel, initializationTimeout); diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpPerformanceTest.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpPerformanceTest.java index 1e2c7449f..488a52f7b 100644 --- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpPerformanceTest.java +++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpPerformanceTest.java @@ -27,6 +27,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; +import java.time.Duration; import java.util.Arrays; import eu.rekawek.toxiproxy.model.ToxicDirection; @@ -38,6 +39,7 @@ import org.apache.sshd.common.keyprovider.KeyIdentityProvider; import org.apache.sshd.sftp.client.SftpClient.OpenMode; import org.apache.sshd.sftp.client.fs.SftpFileSystem; +import org.apache.sshd.util.test.CoreTestSupportUtils; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -48,6 +50,8 @@ @Ignore("Special class used for development only - not really a test just useful to run as such") public class SftpPerformanceTest { + public static final Duration SFTP_CONNECT_TIMEOUT = CoreTestSupportUtils.getTimeout("sftp.connect", Duration.ofSeconds(30)); + public static final Duration SFTP_AUTH_TIMEOUT = CoreTestSupportUtils.getTimeout("sftp.auth", Duration.ofSeconds(15)); public static final String USERNAME = "foo"; public static final String PASSWORD = "pass"; @@ -125,9 +129,11 @@ public ClientSession createClientSession(SshClient client, ContainerProxy proxy) final String ipAddressViaToxiproxy = proxy.getContainerIpAddress(); final int portViaToxiproxy = proxy.getProxyPort(); - ClientSession session = client.connect(USERNAME, ipAddressViaToxiproxy, portViaToxiproxy).verify().getClientSession(); + ClientSession session = client.connect(USERNAME, ipAddressViaToxiproxy, portViaToxiproxy) + .verify(SFTP_CONNECT_TIMEOUT) + .getClientSession(); session.addPasswordIdentity(PASSWORD); - session.auth().verify(); + session.auth().verify(SFTP_AUTH_TIMEOUT); return session; }