You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by lg...@apache.org on 2020/05/19 15:53:18 UTC
[mina-sshd] branch master updated: [SSHD-998] Take into account
SFTP version preference when establishing initial channel
This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
The following commit(s) were added to refs/heads/master by this push:
new a8c7e3b [SSHD-998] Take into account SFTP version preference when establishing initial channel
a8c7e3b is described below
commit a8c7e3b5d3f0525cc02d258fde361e8bd58669e5
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Mon May 18 19:44:38 2020 +0300
[SSHD-998] Take into account SFTP version preference when establishing initial channel
---
CHANGES.md | 7 ++-
docs/sftp.md | 6 +-
.../client/subsystem/sftp/SftpVersionSelector.java | 32 ++++++-----
.../subsystem/sftp/impl/DefaultSftpClient.java | 27 +++++++--
.../sftp/impl/DefaultSftpClientFactory.java | 2 +-
.../subsystem/sftp/SftpSubsystemEnvironment.java | 12 +++-
.../sshd/client/subsystem/sftp/SftpTest.java | 13 +++--
.../subsystem/sftp/SftpVersionSelectorTest.java | 64 +++++++++++++---------
.../subsystem/sftp/fs/SftpFileSystemTest.java | 13 +++--
9 files changed, 111 insertions(+), 65 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 76c083f..c34996c 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -20,6 +20,9 @@ the `SftpFileSystemAccessor` in order to allow easier hooking into the SFTP subs
* Creating files links
* Copying / Renaming / Deleting files
+* `SftpVersionSelector` is now consulted when client sends initial command (as well
+as when session is re-negotiated)
+
## Minor code helpers
* Handling of debug/ignore/unimplemented messages has been split into `handleXXX` and `doInvokeXXXMsgHandler` methods
@@ -48,4 +51,6 @@ as much of the available functionality as possible.
* [SSHD-984](https://issues.apache.org/jira/browse/SSHD-984) - Utility method to export KeyPair in OpenSSH format
-* [SSHD-992](https://issues.apache.org/jira/browse/SSHD-984) - Provide more hooks into the SFTP server subsystem via SftpFileSystemAccessor
\ No newline at end of file
+* [SSHD-992](https://issues.apache.org/jira/browse/SSHD-984) - Provide more hooks into the SFTP server subsystem via SftpFileSystemAccessor
+
+* [SSHD-998](https://issues.apache.org/jira/browse/SSHD-998) - Take into account SFTP version preference when establishing initial channel
\ No newline at end of file
diff --git a/docs/sftp.md b/docs/sftp.md
index ff017be..b7b9469 100644
--- a/docs/sftp.md
+++ b/docs/sftp.md
@@ -184,7 +184,7 @@ range.
SftpVersionSelector myVersionSelector = new SftpVersionSelector() {
@Override
- public int selectVersion(ClientSession session, int current, List<Integer> available) {
+ public int selectVersion(ClientSession session, boolean initial, int current, List<Integer> available) {
int selectedVersion = ...run some logic to decide...;
return selectedVersion;
}
@@ -202,6 +202,10 @@ range.
```
+**Note:** the version selector is invoked **twice** - the first time in order to retrieve the initial version
+to be used when estabilishing the SFTP channel, and the second after having done so after receiving the server's
+version. The invocations are distinguished by the `initial` parameter value.
+
On the server side, version selection restriction is more complex - please remember that according to the
protocol specification
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelector.java b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelector.java
index 58e5efe..98679b4 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelector.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelector.java
@@ -36,29 +36,33 @@ public interface SftpVersionSelector {
/**
* An {@link SftpVersionSelector} that returns the current version
*/
- SftpVersionSelector CURRENT = new NamedVersionSelector("CURRENT", (session, current, available) -> current);
+ SftpVersionSelector CURRENT = new NamedVersionSelector("CURRENT", (session, initial, current, available) -> current);
/**
* An {@link SftpVersionSelector} that returns the maximum available version
*/
SftpVersionSelector MAXIMUM = new NamedVersionSelector(
"MAXIMUM",
- (session, current, available) -> GenericUtils.stream(available).mapToInt(Integer::intValue).max().orElse(current));
+ (session, initial, current, available) -> GenericUtils.stream(available).mapToInt(Integer::intValue).max()
+ .orElse(current));
/**
- * An {@link SftpVersionSelector} that returns the maximum available version
+ * An {@link SftpVersionSelector} that returns the minimum available version
*/
SftpVersionSelector MINIMUM = new NamedVersionSelector(
"MINIMUM",
- (session, current, available) -> GenericUtils.stream(available).mapToInt(Integer::intValue).min().orElse(current));
+ (session, initial, current, available) -> GenericUtils.stream(available).mapToInt(Integer::intValue).min()
+ .orElse(current));
/**
* @param session The {@link ClientSession} through which the SFTP connection is made
+ * @param initial If {@code true} then this is the initial version sent via {@code SSH_FXP_INIT} otherwise it is
+ * a re-negotiation.
* @param current The current version negotiated with the server
* @param available Extra versions available - may be empty and/or contain only the current one
* @return The new requested version - if same as current, then nothing is done
*/
- int selectVersion(ClientSession session, int current, List<Integer> available);
+ int selectVersion(ClientSession session, boolean initial, int current, List<Integer> available);
/**
* Creates a selector the always returns the requested (fixed version) regardless of what the current or reported
@@ -69,13 +73,13 @@ public interface SftpVersionSelector {
* @return The {@link SftpVersionSelector}
*/
static SftpVersionSelector fixedVersionSelector(int version) {
- return new NamedVersionSelector(Integer.toString(version), (session, current, available) -> version);
+ return new NamedVersionSelector(Integer.toString(version), (session, initial, current, available) -> version);
}
/**
* Selects a version in order of preference - if none of the preferred versions is listed as available then an
- * exception is thrown when the {@link SftpVersionSelector#selectVersion(ClientSession, int, List)} method is
- * invoked
+ * exception is thrown when the {@link SftpVersionSelector#selectVersion(ClientSession, boolean, int, List)} method
+ * is invoked
*
* @param preferred The preferred versions in decreasing order of preference (i.e., most preferred is 1st) - may
* not be {@code null}/empty
@@ -88,8 +92,8 @@ public interface SftpVersionSelector {
/**
* Selects a version in order of preference - if none of the preferred versions is listed as available then an
- * exception is thrown when the {@link SftpVersionSelector#selectVersion(ClientSession, int, List)} method is
- * invoked
+ * exception is thrown when the {@link SftpVersionSelector#selectVersion(ClientSession, boolean, int, List)} method
+ * is invoked
*
* @param preferred The preferred versions in decreasing order of preference (i.e., most preferred is 1st)
* @return A {@link SftpVersionSelector} that attempts to select the most preferred version that is also
@@ -99,9 +103,9 @@ public interface SftpVersionSelector {
ValidateUtils.checkNotNullAndNotEmpty((Collection<?>) preferred, "Empty preferred versions");
return new NamedVersionSelector(
GenericUtils.join(preferred, ','),
- (session, current, available) -> StreamSupport.stream(preferred.spliterator(), false)
+ (session, initial, current, available) -> StreamSupport.stream(preferred.spliterator(), false)
.mapToInt(Number::intValue)
- .filter(v -> v == current || available.contains(v))
+ .filter(v -> (v == current) || available.contains(v))
.findFirst()
.orElseThrow(() -> new IllegalStateException(
"Preferred versions (" + preferred + ") not available: " + available)));
@@ -117,8 +121,8 @@ public interface SftpVersionSelector {
}
@Override
- public int selectVersion(ClientSession session, int current, List<Integer> available) {
- return selector.selectVersion(session, current, available);
+ public int selectVersion(ClientSession session, boolean initial, int current, List<Integer> available) {
+ return selector.selectVersion(session, initial, current, available);
}
@Override
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClient.java
index 136503d..7070c8e 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClient.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClient.java
@@ -61,6 +61,7 @@ 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.NullOutputStream;
+import org.apache.sshd.server.subsystem.sftp.SftpSubsystemEnvironment;
/**
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
@@ -77,7 +78,13 @@ public class DefaultSftpClient extends AbstractSftpClient {
private final NavigableMap<String, byte[]> exposedExtensions = Collections.unmodifiableNavigableMap(extensions);
private Charset nameDecodingCharset = DEFAULT_NAME_DECODING_CHARSET;
- public DefaultSftpClient(ClientSession clientSession) throws IOException {
+ /**
+ * @param clientSession The {@link ClientSession}
+ * @param initialVersionSelector The initial {@link SftpVersionSelector} - if {@code null} then version 6 is
+ * assumed.
+ * @throws IOException If failed to initialize
+ */
+ public DefaultSftpClient(ClientSession clientSession, SftpVersionSelector initialVersionSelector) throws IOException {
this.nameDecodingCharset = PropertyResolverUtils.getCharset(
clientSession, NAME_DECODING_CHARSET, DEFAULT_NAME_DECODING_CHARSET);
this.clientSession = Objects.requireNonNull(clientSession, "No client session");
@@ -99,7 +106,7 @@ public class DefaultSftpClient extends AbstractSftpClient {
});
try {
- init(initializationTimeout);
+ init(clientSession, initialVersionSelector, initializationTimeout);
} catch (IOException | RuntimeException e) {
this.channel.close(true);
throw e;
@@ -331,18 +338,26 @@ public class DefaultSftpClient extends AbstractSftpClient {
return null;
}
- protected void init(long initializationTimeout) throws IOException {
+ protected void init(ClientSession session, SftpVersionSelector initialVersionSelector, long initializationTimeout)
+ throws IOException {
+ int initialVersion = (initialVersionSelector == null)
+ ? SftpConstants.SFTP_V6
+ : initialVersionSelector.selectVersion(
+ session, true, SftpConstants.SFTP_V6, SftpSubsystemEnvironment.SUPPORTED_SFTP_VERSIONS);
+ ValidateUtils.checkState(SftpSubsystemEnvironment.SUPPORTED_SFTP_VERSIONS.contains(initialVersion),
+ "Unsupported initial version selected: %d", initialVersion);
+
// Send init packet
Buffer buf = new ByteArrayBuffer(INIT_COMMAND_SIZE + SshConstants.SSH_PACKET_HEADER_LEN);
buf.putInt(INIT_COMMAND_SIZE);
buf.putByte((byte) SftpConstants.SSH_FXP_INIT);
- buf.putInt(SftpConstants.SFTP_V6);
+ buf.putInt(initialVersion);
boolean traceEnabled = log.isTraceEnabled();
IoOutputStream asyncIn = channel.getAsyncIn();
ClientChannel clientChannel = getClientChannel();
if (traceEnabled) {
- log.trace("init({}) send SSH_FXP_INIT", clientChannel);
+ log.trace("init({}) send SSH_FXP_INIT - initial version={}", clientChannel, initialVersion);
}
IoWriteFuture writeFuture = asyncIn.writePacket(buf);
writeFuture.verify();
@@ -480,7 +495,7 @@ public class DefaultSftpClient extends AbstractSftpClient {
}
ClientSession session = getClientSession();
- int selected = selector.selectVersion(session, current, availableVersions);
+ int selected = selector.selectVersion(session, false, current, availableVersions);
if (debugEnabled) {
log.debug("negotiateVersion({}) current={} {} -> {}",
clientChannel, current, availableVersions, selected);
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClientFactory.java b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClientFactory.java
index 4d910b5..de6c05c 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClientFactory.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/DefaultSftpClientFactory.java
@@ -65,7 +65,7 @@ public class DefaultSftpClientFactory extends AbstractLoggingBean implements Sft
protected DefaultSftpClient createDefaultSftpClient(ClientSession session, SftpVersionSelector selector)
throws IOException {
- return new DefaultSftpClient(session);
+ return new DefaultSftpClient(session, selector);
}
@Override
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemEnvironment.java b/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemEnvironment.java
index 8561018..3480612 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemEnvironment.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemEnvironment.java
@@ -20,11 +20,14 @@
package org.apache.sshd.server.subsystem.sftp;
import java.nio.file.Path;
+import java.util.Collections;
+import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.apache.sshd.common.session.SessionHolder;
import org.apache.sshd.common.subsystem.sftp.SftpConstants;
+import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.server.config.SshServerConfigFileReader;
import org.apache.sshd.server.session.ServerSession;
import org.apache.sshd.server.session.ServerSessionHolder;
@@ -42,9 +45,12 @@ public interface SftpSubsystemEnvironment extends SessionHolder<ServerSession>,
int HIGHER_SFTP_IMPL = SftpConstants.SFTP_V6; // .. up to and including
- String ALL_SFTP_IMPL = IntStream.rangeClosed(LOWER_SFTP_IMPL, HIGHER_SFTP_IMPL)
- .mapToObj(Integer::toString)
- .collect(Collectors.joining(","));
+ List<Integer> SUPPORTED_SFTP_VERSIONS = Collections.unmodifiableList(
+ IntStream.rangeClosed(LOWER_SFTP_IMPL, HIGHER_SFTP_IMPL)
+ .boxed()
+ .collect(Collectors.toList()));
+
+ String ALL_SFTP_IMPL = GenericUtils.join(SUPPORTED_SFTP_VERSIONS, ',');
@Override
default ServerSession getSession() {
diff --git a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
index d5484ad..f4e982c 100644
--- a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
+++ b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
@@ -1330,12 +1330,13 @@ public class SftpTest extends AbstractSftpClientTestSupport {
@Test
public void testSftpVersionSelector() throws Exception {
AtomicInteger selected = new AtomicInteger(-1);
- SftpVersionSelector selector = (session, current, available) -> {
- int value = GenericUtils.stream(available)
- .mapToInt(Integer::intValue)
- .filter(v -> v != current)
- .max()
- .orElseGet(() -> current);
+ SftpVersionSelector selector = (session, initial, current, available) -> {
+ int value = initial
+ ? current : GenericUtils.stream(available)
+ .mapToInt(Integer::intValue)
+ .filter(v -> v != current)
+ .max()
+ .orElseGet(() -> current);
selected.set(value);
return value;
};
diff --git a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelectorTest.java b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelectorTest.java
index 46efd69..5e0f625 100644
--- a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelectorTest.java
+++ b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionSelectorTest.java
@@ -53,18 +53,23 @@ public class SftpVersionSelectorTest extends JUnitTestSupport {
for (int expected = SftpSubsystemEnvironment.LOWER_SFTP_IMPL;
expected <= SftpSubsystemEnvironment.HIGHER_SFTP_IMPL;
expected++) {
- assertEquals("Mismatched directly selected for available=" + available, expected,
- SftpVersionSelector.CURRENT.selectVersion(session, expected, available));
- available.add(expected);
+ for (boolean initial : new boolean[] { true, false }) {
+ assertEquals("Mismatched directly selected for initial=" + initial + "/available=" + available, expected,
+ SftpVersionSelector.CURRENT.selectVersion(session, initial, expected, available));
+ available.add(expected);
+ }
}
for (int expected = SftpSubsystemEnvironment.LOWER_SFTP_IMPL;
expected <= SftpSubsystemEnvironment.HIGHER_SFTP_IMPL;
expected++) {
- for (int index = 0; index < available.size(); index++) {
- Collections.shuffle(available, rnd);
- assertEquals("Mismatched suffling selected for current=" + expected + ", available=" + available,
- expected, SftpVersionSelector.CURRENT.selectVersion(session, expected, available));
+ for (boolean initial : new boolean[] { true, false }) {
+ for (int index = 0; index < available.size(); index++) {
+ Collections.shuffle(available, rnd);
+ assertEquals("Mismatched suffling selected for initial=" + initial + ", current=" + expected
+ + ", available=" + available,
+ expected, SftpVersionSelector.CURRENT.selectVersion(session, initial, expected, available));
+ }
}
}
}
@@ -93,23 +98,26 @@ public class SftpVersionSelectorTest extends JUnitTestSupport {
SftpVersionSelector selector = SftpVersionSelector.preferredVersionSelector(preferred);
int expected = preferred.get(0);
- for (int current = SftpSubsystemEnvironment.LOWER_SFTP_IMPL;
- current <= SftpSubsystemEnvironment.HIGHER_SFTP_IMPL;
- current++) {
- assertEquals(
- "Mismatched selected for current= " + current + ", available=" + available + ", preferred=" + preferred,
- expected, selector.selectVersion(session, current, available));
+ for (boolean initial : new boolean[] { true, false }) {
+ for (int current = SftpSubsystemEnvironment.LOWER_SFTP_IMPL;
+ current <= SftpSubsystemEnvironment.HIGHER_SFTP_IMPL;
+ current++) {
+ assertEquals(
+ "Mismatched selected for current= " + current + ", available=" + available + ", preferred="
+ + preferred,
+ expected, selector.selectVersion(session, initial, current, available));
- try {
- Collections.shuffle(unavailable, rnd);
- int version = unavailable.get(0);
- int actual = selector.selectVersion(session, version, unavailable);
- fail("Unexpected selected version (" + actual + ")"
- + " for current= " + version
- + ", available=" + unavailable
- + ", preferred=" + preferred);
- } catch (IllegalStateException e) {
- // expected
+ try {
+ Collections.shuffle(unavailable, rnd);
+ int version = unavailable.get(0);
+ int actual = selector.selectVersion(session, initial, version, unavailable);
+ fail("Unexpected selected version (" + actual + ")"
+ + " for current= " + version
+ + ", available=" + unavailable
+ + ", preferred=" + preferred);
+ } catch (IllegalStateException e) {
+ // expected
+ }
}
}
}
@@ -138,10 +146,12 @@ public class SftpVersionSelectorTest extends JUnitTestSupport {
for (int current = SftpSubsystemEnvironment.LOWER_SFTP_IMPL;
current <= SftpSubsystemEnvironment.HIGHER_SFTP_IMPL;
current++) {
- for (int index = 0; index < available.size(); index++) {
- assertEquals("Mismatched selection for current=" + current + ", available=" + available,
- expected, selector.selectVersion(session, current, available));
- Collections.shuffle(available, rnd);
+ for (boolean initial : new boolean[] { true, false }) {
+ for (int index = 0; index < available.size(); index++) {
+ assertEquals("Mismatched selection for current=" + current + ", available=" + available,
+ expected, selector.selectVersion(session, initial, current, available));
+ Collections.shuffle(available, rnd);
+ }
}
}
}
diff --git a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/fs/SftpFileSystemTest.java b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/fs/SftpFileSystemTest.java
index 517fef4..484ade2 100644
--- a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/fs/SftpFileSystemTest.java
+++ b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/fs/SftpFileSystemTest.java
@@ -329,12 +329,13 @@ public class SftpFileSystemTest extends BaseTestSupport {
@Test
public void testSftpVersionSelector() throws Exception {
AtomicInteger selected = new AtomicInteger(-1);
- SftpVersionSelector selector = (session, current, available) -> {
- int value = GenericUtils.stream(available)
- .mapToInt(Integer::intValue)
- .filter(v -> v != current)
- .max()
- .orElseGet(() -> current);
+ SftpVersionSelector selector = (session, initial, current, available) -> {
+ int value = initial
+ ? current : GenericUtils.stream(available)
+ .mapToInt(Integer::intValue)
+ .filter(v -> v != current)
+ .max()
+ .orElseGet(() -> current);
selected.set(value);
return value;
};