You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by tw...@apache.org on 2024/08/06 19:30:12 UTC
(mina-sshd) branch master updated (d53fea977 -> 16b307865)
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
from d53fea977 GH-524: More ChaCha20-Poly1305 optimizations
new c2a476094 GH-524: Avoid unnecessary work in SFTP uploads
new 9e4e040ed Clean-up: timeout-free KEX
new 16b307865 Resolve warning from maven-dependency-plugin
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
pom.xml | 2 +-
.../common/session/helpers/AbstractSession.java | 35 ++++++++++++-------
.../org/apache/sshd/core/CoreModuleProperties.java | 11 +++---
.../sftp/client/impl/SftpOutputStreamAsync.java | 39 +++++++++++++++++++---
.../apache/sshd/sftp/client/impl/SftpResponse.java | 3 +-
.../apache/sshd/sftp/client/impl/SftpStatus.java | 8 ++++-
6 files changed, 73 insertions(+), 25 deletions(-)
(mina-sshd) 02/03: Clean-up: timeout-free KEX
Posted by tw...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 9e4e040edebab7eb3616dabc382b742714b9f580
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Tue Aug 6 20:31:26 2024 +0200
Clean-up: timeout-free KEX
Do not wait synchronously until the kexInitializedFuture is fulfilled.
Instead simply submit the negotiation as a listener on that future.
It'll run once our own KEX proposal has been composed.
Waiting synchronously has the potential to block if the MINA transport
is used.
Deprecate CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT; it's unused
now.
---
.../common/session/helpers/AbstractSession.java | 35 ++++++++++++++--------
.../org/apache/sshd/core/CoreModuleProperties.java | 11 ++++---
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
index 31589a4bf..023ea777b 100644
--- a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
+++ b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
@@ -890,25 +890,36 @@ public abstract class AbstractSession extends SessionHelper {
sendKexInit();
break;
case BOTH:
- DefaultKeyExchangeFuture initFuture;
- synchronized (kexState) {
- initFuture = kexInitializedFuture;
- if (initFuture == null) {
- initFuture = new DefaultKeyExchangeFuture(toString(), null);
- kexInitializedFuture = initFuture;
- }
- }
- // requestNewKeyExchange() is running in some other thread: wait until it has set up our own proposal.
- // The timeout is a last resort only to avoid blocking indefinitely in case something goes
- // catastrophically wrong somewhere; it should never be hit. If it is, an exception will be thrown.
+ // We are in the process of sending our own KEX_INIT. Do the negotiation once that's done.
//
// See https://issues.apache.org/jira/browse/SSHD-1197
- initFuture.await(CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this));
break;
default:
throw new IllegalStateException("Received SSH_MSG_KEXINIT while key exchange is running");
}
+ // Note: we should not wait here; it might block (in particular with the MINA transport back-end).
+ DefaultKeyExchangeFuture initFuture;
+ synchronized (kexState) {
+ initFuture = kexInitializedFuture;
+ if (initFuture == null) {
+ initFuture = new DefaultKeyExchangeFuture(toString(), null);
+ kexInitializedFuture = initFuture;
+ }
+ }
+ initFuture.addListener(f -> {
+ if (f.isDone()) {
+ try {
+ performKexNegotiation();
+ } catch (Exception e) {
+ exceptionCaught(e);
+ }
+ } else {
+ exceptionCaught(f.getException());
+ }
+ });
+ }
+ protected void performKexNegotiation() throws Exception {
Map<KexProposalOption, String> result = negotiate();
String kexAlgorithm = result.get(KexProposalOption.ALGORITHMS);
Collection<? extends KeyExchangeFactory> kexFactories = getKeyExchangeFactories();
diff --git a/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java b/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java
index d8c8d35c1..d130648d5 100644
--- a/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java
+++ b/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java
@@ -138,14 +138,13 @@ public final class CoreModuleProperties {
= Property.bool("allow-dhg1-kex-fallback", false);
/**
- * If the peer initiates a key exchange, we send our own KEX_INIT message with the proposal. This is a last-resort
- * timeout for waiting until we have prepared our own KEX proposal. This timeout should actually never be hit unless
- * there is a serious deadlock somewhere and the session is never closed. It should be set to a reasonably high
- * value; it must be at least 5 seconds and the default is 42 seconds. If the timeout is ever hit, the key exchange
- * initiated by the peer will fail.
+ * Unused.
+ *
+ * @deprecated since 2.14.0
*/
+ @Deprecated
public static final Property<Duration> KEX_PROPOSAL_SETUP_TIMEOUT
- = Property.durationSec("kex-proposal-setup-timeout", Duration.ofSeconds(42), Duration.ofSeconds(5));
+ = Property.duration("kex-proposal-setup-timeout", Duration.ZERO);
/**
* Key used to set the heartbeat interval in milliseconds (0 to disable = default)
(mina-sshd) 03/03: Resolve warning from maven-dependency-plugin
Posted by tw...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 16b3078658b5fab324033b9d6d7d46393210647d
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Tue Aug 6 21:19:49 2024 +0200
Resolve warning from maven-dependency-plugin
Use "resolve-sources" instead of the deprecated "sources" goal.
---
pom.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pom.xml b/pom.xml
index 65a458328..195587cc9 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1178,7 +1178,7 @@
<id>download-sources</id>
<phase>install</phase>
<goals> <!-- automatically download the dependency sources - useful for debugging -->
- <goal>sources</goal>
+ <goal>resolve-sources</goal>
</goals>
<configuration>
<!-- no need to download our own sources - especially snapshot -->
(mina-sshd) 01/03: GH-524: Avoid unnecessary work in SFTP uploads
Posted by tw...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit c2a476094e2119971aa07be457e8a0aa1c0a3ec6
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Tue Aug 6 20:15:51 2024 +0200
GH-524: Avoid unnecessary work in SFTP uploads
Streamline ACK checking a bit. Do a quick check for the OK case first
and do the full and proper parsing only if the status is not OK. Also
avoid getting the IDLE_TIMEOUT repeatedly when draining remaining ACKs
when SftpOutputStreamAsync is closed.
---
.../sftp/client/impl/SftpOutputStreamAsync.java | 39 +++++++++++++++++++---
.../apache/sshd/sftp/client/impl/SftpResponse.java | 3 +-
.../apache/sshd/sftp/client/impl/SftpStatus.java | 8 ++++-
3 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpOutputStreamAsync.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpOutputStreamAsync.java
index bd9200083..c35c05d6e 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpOutputStreamAsync.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpOutputStreamAsync.java
@@ -32,16 +32,19 @@ import java.util.Objects;
import org.apache.sshd.common.SshConstants;
import org.apache.sshd.common.session.Session;
import org.apache.sshd.common.session.helpers.PacketBuffer;
+import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.ValidateUtils;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
import org.apache.sshd.common.util.io.output.OutputStreamWithChannel;
+import org.apache.sshd.core.CoreModuleProperties;
import org.apache.sshd.sftp.client.SftpClient;
import org.apache.sshd.sftp.client.SftpClient.CloseableHandle;
import org.apache.sshd.sftp.client.SftpClient.OpenMode;
import org.apache.sshd.sftp.client.SftpClientHolder;
import org.apache.sshd.sftp.client.SftpMessage;
import org.apache.sshd.sftp.common.SftpConstants;
+import org.apache.sshd.sftp.common.SftpException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -327,8 +330,7 @@ public class SftpOutputStreamAsync extends OutputStreamWithChannel implements Sf
}
pendingAcks.removeFirst();
- SftpResponse response = SftpResponse.parse(SftpConstants.SSH_FXP_WRITE, buf);
- client.checkResponseStatus(SftpConstants.SSH_FXP_WRITE, ack.id, SftpStatus.parse(response));
+ checkStatus(client, buf);
}
if (buffer == null) {
@@ -366,6 +368,27 @@ public class SftpOutputStreamAsync extends OutputStreamWithChannel implements Sf
buffer = null;
}
+ private void checkStatus(AbstractSftpClient client, Buffer buf) throws IOException {
+ if (buf.available() >= 13) {
+ int rpos = buf.rpos();
+ buf.rpos(rpos + 4); // Skip length
+ int cmd = buf.getUByte();
+ if (cmd != SftpConstants.SSH_FXP_STATUS) {
+ throw new SftpException(SftpConstants.SSH_FX_BAD_MESSAGE,
+ "Unexpected SFTP response; expected SSH_FXP_STATUS but got "
+ + SftpConstants.getCommandMessageName(cmd));
+ }
+ buf.rpos(rpos + 9); // Skip ahead until after the id
+ if (buf.getInt() == SftpConstants.SSH_FX_OK) {
+ return;
+ }
+ // Reset and do the full parse
+ buf.rpos(rpos);
+ }
+ SftpResponse response = SftpResponse.parse(SftpConstants.SSH_FXP_WRITE, buf);
+ client.checkResponseStatus(SftpConstants.SSH_FXP_WRITE, response.getId(), SftpStatus.parse(response));
+ }
+
@Override
public void close() throws IOException {
if (!isOpen()) {
@@ -388,6 +411,10 @@ public class SftpOutputStreamAsync extends OutputStreamWithChannel implements Sf
lastMsg = null;
}
+ Duration idleTimeout = CoreModuleProperties.IDLE_TIMEOUT.getRequired(getClient().getClientSession());
+ if (GenericUtils.isNegativeOrNull(idleTimeout)) {
+ idleTimeout = CoreModuleProperties.IDLE_TIMEOUT.getRequiredDefault();
+ }
AbstractSftpClient client = getClient();
for (int ackIndex = 1; !pendingAcks.isEmpty(); ackIndex++) {
SftpAckData ack = pendingAcks.removeFirst();
@@ -395,11 +422,15 @@ public class SftpOutputStreamAsync extends OutputStreamWithChannel implements Sf
log.debug("close({}) processing ack #{}: {}", this, ackIndex, ack);
}
- SftpResponse response = client.response(SftpConstants.SSH_FXP_WRITE, ack.id);
+ Buffer buf = client.receive(ack.id, idleTimeout);
+ if (buf == null) {
+ log.debug("close({}) no ack response for {}", this, ack);
+ break;
+ }
if (debugEnabled) {
log.debug("close({}) processing ack #{} response for {}", this, ackIndex, ack);
}
- client.checkResponseStatus(SftpConstants.SSH_FXP_WRITE, response.getId(), SftpStatus.parse(response));
+ checkStatus(client, buf);
}
} finally {
if (ownsHandle) {
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpResponse.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpResponse.java
index fd8787d9f..3ce0839fc 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpResponse.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpResponse.java
@@ -71,7 +71,8 @@ public final class SftpResponse {
int length = buffer.getInt();
int type = buffer.getUByte();
int id = buffer.getInt();
- validateIncomingResponse(cmd, id, type, length, buffer);
+ // No need to validate the length here: the way we assemble these buffers guarantees that
+ // the length is reasonable and does not exceed buffer.available().
return new SftpResponse(cmd, id, type, length, buffer);
}
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpStatus.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpStatus.java
index 5d1511fcf..93fea737d 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpStatus.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpStatus.java
@@ -20,6 +20,7 @@ package org.apache.sshd.sftp.client.impl;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.sftp.common.SftpConstants;
+import org.apache.sshd.sftp.common.SftpException;
/**
* A representation of a SSH_FXP_STATUS record.
@@ -69,7 +70,12 @@ public final class SftpStatus {
return new SftpStatus(code, message, language);
}
- public static SftpStatus parse(SftpResponse response) {
+ public static SftpStatus parse(SftpResponse response) throws SftpException {
+ if (response.getType() != SftpConstants.SSH_FXP_STATUS) {
+ throw new SftpException(
+ SftpConstants.SSH_FX_BAD_MESSAGE, "Unexpected SFTP response: expected SSH_FXP_STATUS but got "
+ + SftpConstants.getCommandMessageName(response.getType()));
+ }
return parse(response.getBuffer());
}
}