Repository: mina-sshd Updated Branches: refs/heads/master 318a7ff4f -> 54b56b844
Fixed SFTP handling of links - symbolic (especially) and hard ones (untested) Project: http://git-wip-us.apache.org/repos/asf/mina-sshd/repo Commit: http://git-wip-us.apache.org/repos/asf/mina-sshd/commit/54b56b84 Tree: http://git-wip-us.apache.org/repos/asf/mina-sshd/tree/54b56b84 Diff: http://git-wip-us.apache.org/repos/asf/mina-sshd/diff/54b56b84 Branch: refs/heads/master Commit: 54b56b844bc057c36ef7d8ebebd0df14e8b44fc7 Parents: 318a7ff Author: Lyor Goldstein <[email protected]> Authored: Sun Jul 19 15:57:58 2015 +0300 Committer: Lyor Goldstein <[email protected]> Committed: Sun Jul 19 15:57:58 2015 +0300 ---------------------------------------------------------------------- .../subsystem/sftp/SftpFileSystemProvider.java | 2 +- .../file/root/RootedFileSystemProvider.java | 77 ++++++++----- .../sshd/server/subsystem/sftp/SftpHelper.java | 3 +- .../server/subsystem/sftp/SftpSubsystem.java | 111 +++++++++++++------ .../subsystem/sftp/SftpFileSystemTest.java | 24 ++-- .../sshd/client/subsystem/sftp/SftpTest.java | 35 ++++-- 6 files changed, 161 insertions(+), 91 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemProvider.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemProvider.java b/sshd-core/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemProvider.java index 9bba23e..87579c5 100644 --- a/sshd-core/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemProvider.java +++ b/sshd-core/src/main/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemProvider.java @@ -448,7 +448,7 @@ public class SftpFileSystemProvider extends FileSystemProvider { // attributes of source file BasicFileAttributes attrs = readAttributes(source, BasicFileAttributes.class, linkOptions); if (attrs.isSymbolicLink()) { - throw new IOException("Copying of symbolic links not supported"); + throw new IOException("Moving of source symbolic link not supported: " + source); } // delete target if it exists and REPLACE_EXISTING is specified http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java b/sshd-core/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java index 057e112..458667b 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java @@ -191,18 +191,32 @@ public class RootedFileSystemProvider extends FileSystemProvider { @Override public void createSymbolicLink(Path link, Path target, FileAttribute<?>... attrs) throws IOException { - Path l = unroot(link); - Path t = unroot(target, false); - FileSystemProvider p = provider(l); - p.createSymbolicLink(l, t, attrs); + createLink(link, target, true, attrs); } @Override public void createLink(Path link, Path existing) throws IOException { + createLink(link, existing, false); + } + + protected void createLink(Path link, Path target, boolean symLink, FileAttribute<?>... attrs) throws IOException { Path l = unroot(link); - Path e = unroot(existing); + Path t = unroot(target); + /* + * For a symbolic link preserve the relative path + */ + if (symLink && (!target.isAbsolute())) { + RootedFileSystem rfs = ((RootedPath) target).getFileSystem(); + Path root = rfs.getRoot(); + t = root.relativize(t); + } + FileSystemProvider p = provider(l); - p.createLink(l, e); + if (symLink) { + p.createSymbolicLink(l, t, attrs); + } else { + p.createLink(l, t); + } } @Override @@ -223,7 +237,7 @@ public class RootedFileSystemProvider extends FileSystemProvider { public Path readSymbolicLink(Path link) throws IOException { Path r = unroot(link); FileSystemProvider p = provider(r); - return root(link.getFileSystem(), p.readSymbolicLink(r)); + return root((RootedFileSystem) link.getFileSystem(), p.readSymbolicLink(r)); } @Override @@ -265,7 +279,7 @@ public class RootedFileSystemProvider extends FileSystemProvider { } protected RootedFileSystem getFileSystem(Path path) throws FileSystemNotFoundException { - Path real = unroot(path, false); + Path real = unroot(path); Path rootInstance = null; RootedFileSystem fsInstance = null; synchronized (fileSystems) { @@ -330,13 +344,12 @@ public class RootedFileSystemProvider extends FileSystemProvider { p.setAttribute(r, attribute, value, options); } - private static FileSystemProvider provider(Path path) { + protected FileSystemProvider provider(Path path) { FileSystem fs = path.getFileSystem(); return fs.provider(); } - private static Path root(FileSystem fs, Path nat) { - RootedFileSystem rfs = (RootedFileSystem) fs; + protected Path root(RootedFileSystem rfs, Path nat) { if (nat.isAbsolute()) { Path root = rfs.getRoot(); Path rel = root.relativize(nat); @@ -346,30 +359,34 @@ public class RootedFileSystemProvider extends FileSystemProvider { } } - private static Path unroot(Path path) { - return unroot(path, true); - } - - private static Path unroot(Path path, boolean absolute) { + /** + * @param path The original (rooted) {@link Path} + * @return The actual <U>absolute <B>local</B></U> {@link Path} represented + * by the rooted one + * @see #resolveLocalPath(RootedPath) + * @throws IllegalArgumentException if {@code null} path argument + * @throws ProviderMismatchException if not a {@link RootedPath} + */ + protected Path unroot(Path path) { ValidateUtils.checkNotNull(path, "No path to unroot"); if (!(path instanceof RootedPath)) { throw new ProviderMismatchException("unroot(" + path + ") is not a " + RootedPath.class.getSimpleName() + " but rather a " + path.getClass().getSimpleName()); } - RootedPath p = (RootedPath) path; - if (absolute || p.isAbsolute()) { - Path absPath = p.toAbsolutePath(); - String r = absPath.toString(); - RootedFileSystem rfs = p.getFileSystem(); - Path root = rfs.getRoot(); - return root.resolve(r.substring(1)); - } else { - RootedPath fileName = p.getFileName(); - RootedPath root = fileName.getRoot(); - RootedFileSystem rfs = root.getFileSystem(); - return rfs.getPath(p.toString()); - } + return resolveLocalPath((RootedPath) path); + } + + /** + * @param path The original {@link RootedPath} - never {@code null} + * @return The actual <U>absolute <B>local</B></U> {@link Path} represented + * by the rooted one + */ + protected Path resolveLocalPath(RootedPath path) { + Path absPath = path.toAbsolutePath(); + String r = absPath.toString(); + RootedFileSystem rfs = path.getFileSystem(); + Path root = rfs.getRoot(); + return root.resolve(r.substring(1)); } - } http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpHelper.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpHelper.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpHelper.java index a8a8315..060bb88 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpHelper.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpHelper.java @@ -125,7 +125,7 @@ import static org.apache.sshd.common.subsystem.sftp.SftpConstants.S_IXUSR; /** * @author <a href="mailto:[email protected]";>Apache MINA SSHD Project</a> */ -final class SftpHelper { +public final class SftpHelper { private SftpHelper() { throw new UnsupportedOperationException("No instance allowed"); @@ -140,6 +140,7 @@ final class SftpHelper { throw new IllegalStateException("Unsupported SFTP version: " + version); } } + public static void writeAttrsV3(Buffer buffer, Map<String, ?> attributes) throws IOException { boolean isReg = getBool((Boolean) attributes.get("isRegularFile")); boolean isDir = getBool((Boolean) attributes.get("isDirectory")); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java index 3805b29..fa63181 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java @@ -54,6 +54,7 @@ import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -391,7 +392,11 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna public void setFileSystem(FileSystem fileSystem) { if (fileSystem != this.fileSystem) { this.fileSystem = fileSystem; - this.defaultDir = fileSystem.getRootDirectories().iterator().next(); + + Iterable<Path> roots = ValidateUtils.checkNotNull(fileSystem.getRootDirectories(), "No root directories"); + Iterator<Path> available = ValidateUtils.checkNotNull(roots.iterator(), "No roots iterator"); + ValidateUtils.checkTrue(available.hasNext(), "No available root"); + this.defaultDir = available.next(); } } @@ -621,17 +626,14 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected SpaceAvailableExtensionInfo doSpaceAvailable(int id, String path) throws IOException { - Path file = resolveFile(path); - Path abs = file.toAbsolutePath(); - Path nrm = abs.normalize(); + Path nrm = resolveNormalizedLocation(path); if (log.isDebugEnabled()) { log.debug("doSpaceAvailable(id={}) path={}[{}]", id, path, nrm); } FileStore store = Files.getFileStore(nrm); if (log.isTraceEnabled()) { - log.trace("doSpaceAvailable(id={}) path={}[{}] - {}[{}]", - id, path, nrm, store.name(), store.type()); + log.trace("doSpaceAvailable(id={}) path={}[{}] - {}[{}]", id, path, nrm, store.name(), store.type()); } return new SpaceAvailableExtensionInfo(store); @@ -1182,6 +1184,11 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna boolean symLink = buffer.getBoolean(); try { + if (log.isDebugEnabled()) { + log.debug("Received SSH_FXP_LINK id={}, linkpath={}, targetpath={}, symlink={}", + id, linkPath, targetPath, symLink); + } + doLink(id, targetPath, linkPath, symLink); } catch (IOException | RuntimeException e) { sendStatus(BufferUtils.clear(buffer), id, e); @@ -1192,24 +1199,16 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected void doLink(int id, String targetPath, String linkPath, boolean symLink) throws IOException { - if (log.isDebugEnabled()) { - log.debug("Received SSH_FXP_LINK (linkpath={}, targetpath={}, symlink={})", - linkPath, targetPath, symLink); - } - - Path link = resolveFile(linkPath); - Path target = fileSystem.getPath(targetPath); - if (symLink) { - Files.createSymbolicLink(link, target); - } else { - Files.createLink(link, target); - } + createLink(id, targetPath, linkPath, symLink); } protected void doSymLink(Buffer buffer, int id) throws IOException { String targetPath = buffer.getString(); String linkPath = buffer.getString(); try { + if (log.isDebugEnabled()) { + log.debug("Received SSH_FXP_SYMLINK id={}, linkpath={}, targetpath={}", id, targetPath, linkPath); + } doSymLink(id, targetPath, linkPath); } catch (IOException | RuntimeException e) { sendStatus(BufferUtils.clear(buffer), id, e); @@ -1220,16 +1219,31 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected void doSymLink(int id, String targetPath, String linkPath) throws IOException { - log.debug("Received SSH_FXP_SYMLINK (linkpath={}, targetpath={})", targetPath, linkPath); + createLink(id, targetPath, linkPath, true); + } + + protected void createLink(int id, String targetPath, String linkPath, boolean symLink) throws IOException { Path link = resolveFile(linkPath); Path target = fileSystem.getPath(targetPath); - Files.createSymbolicLink(link, target); + if (log.isDebugEnabled()) { + log.debug("createLink(id={}), linkpath={}[{}], targetpath={}[{}], symlink={})", + id, linkPath, link, targetPath, target, symLink); + } + + if (symLink) { + Files.createSymbolicLink(link, target); + } else { + Files.createLink(link, target); + } } protected void doReadLink(Buffer buffer, int id) throws IOException { String path = buffer.getString(); String l; try { + if (log.isDebugEnabled()) { + log.debug("Received SSH_FXP_READLINK id={} path={}", id, path); + } l = doReadLink(id, path); } catch (IOException | RuntimeException e) { sendStatus(BufferUtils.clear(buffer), id, e); @@ -1241,9 +1255,10 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna protected String doReadLink(int id, String path) throws IOException { Path f = resolveFile(path); - log.debug("Received SSH_FXP_READLINK (path={}[{}])", path, f); - Path t = Files.readSymbolicLink(f); + if (log.isDebugEnabled()) { + log.debug("doReadLink(id={}) path={}[{}]: {}", id, path, f, t); + } return t.toString(); } @@ -1546,8 +1561,7 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna * @see IoUtils#checkFileExists(Path, LinkOption...) */ protected Pair<Path, Boolean> validateRealPath(int id, String path, Path f, LinkOption... options) throws IOException { - Path abs = f.toAbsolutePath(); - Path p = abs.normalize(); + Path p = normalize(f); Boolean status = IoUtils.checkFileExists(p, options); return new Pair<>(p, status); } @@ -1718,9 +1732,7 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected String doOpenDir(int id, String path, LinkOption... options) throws IOException { - Path f = resolveFile(path); - Path abs = f.toAbsolutePath(); - Path p = abs.normalize(); + Path p = resolveNormalizedLocation(path); log.debug("Received SSH_FXP_OPENDIR (path={})[{}]", path, p); Boolean status = IoUtils.checkFileExists(p, options); @@ -2309,13 +2321,24 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected void sendLink(Buffer buffer, int id, String link) throws IOException { + //in case we are running on Windows + String unixPath = link.replace(File.separatorChar, '/'); buffer.putByte((byte) SSH_FXP_NAME); buffer.putInt(id); - buffer.putInt(1); - //normalize the given path, use *nix style separator - buffer.putString(link); - buffer.putString(link); - buffer.putInt(0); + buffer.putInt(1); // one response + + buffer.putString(unixPath); + if (version == SFTP_V3) { + buffer.putString(unixPath); + } + + /* + * As per the spec: + * + * The server will respond with a SSH_FXP_NAME packet containing only + * one name and a dummy attributes value. + */ + SftpHelper.writeAttrs(version, buffer, Collections.<String, Object>emptyMap()); send(buffer); } @@ -2442,8 +2465,7 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } protected String getShortName(Path f) throws IOException { - Path abs = f.toAbsolutePath(); - Path nrm = abs.normalize(); + Path nrm = normalize(f); int count = nrm.getNameCount(); /* * According to the javadoc: @@ -2890,13 +2912,30 @@ public class SftpSubsystem extends AbstractLoggingBean implements Command, Runna } } + protected Path resolveNormalizedLocation(String remotePath) throws IOException, InvalidPathException { + return normalize(resolveFile(remotePath)); + } + + protected Path normalize(Path f) { + if (f == null) { + return null; + } + + Path abs = f.isAbsolute() ? f : f.toAbsolutePath(); + return abs.normalize(); + } + protected Path resolveFile(String remotePath) throws IOException, InvalidPathException { // In case double slashes and other patterns are used String path = SelectorUtils.normalizePath(remotePath, "/"); String localPath = SelectorUtils.translateToLocalPath(path); // In case we are running on Windows - return defaultDir.resolve(localPath); + Path p = defaultDir.resolve(localPath); + if (log.isTraceEnabled()) { + log.trace("resolveFile({}) {}", remotePath, p); + } + + return p; } - } http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemTest.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemTest.java b/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemTest.java index 99777b5..1623d98 100644 --- a/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpFileSystemTest.java @@ -382,18 +382,18 @@ public class SftpFileSystemTest extends BaseTestSupport { // assertTrue(Files.isSymbolicLink(link)); // assertEquals("test.txt", Files.readSymbolicLink(link).toString()); - // TODO there are many issues with symbolic links Windows and Linux - for now they are of a lesser interest -// if (OsUtils.isUNIX()) { -// Path link = fs.getPath(remFile2Path); -// Path linkParent = link.getParent(); -// Path relPath = linkParent.relativize(file1); -// Files.createSymbolicLink(link, relPath); -// assertTrue("Not a symbolic link: " + link, Files.isSymbolicLink(link)); -// -// Path symLink = Files.readSymbolicLink(link); -// assertEquals("mismatched symbolic link name", relPath.toString(), symLink.toString()); -// Files.delete(link); -// } + // TODO there are many issues with symbolic links on Windows + if (OsUtils.isUNIX()) { + Path link = fs.getPath(remFile2Path); + Path linkParent = link.getParent(); + Path relPath = linkParent.relativize(file1); + Files.createSymbolicLink(link, relPath); + assertTrue("Not a symbolic link: " + link, Files.isSymbolicLink(link)); + + Path symLink = Files.readSymbolicLink(link); + assertEquals("mismatched symbolic link name", relPath.toString(), symLink.toString()); + Files.delete(link); + } attrs = Files.readAttributes(file1, "*", LinkOption.NOFOLLOW_LINKS); System.out.append(file1.toString()).append(" no-follow attributes: ").println(attrs); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/54b56b84/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java b/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java index 70481de..82e5578 100644 --- a/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java @@ -18,6 +18,11 @@ */ package org.apache.sshd.client.subsystem.sftp; +import static org.apache.sshd.common.subsystem.sftp.SftpConstants.SSH_FX_FILE_ALREADY_EXISTS; +import static org.apache.sshd.common.subsystem.sftp.SftpConstants.SSH_FX_NO_SUCH_FILE; +import static org.apache.sshd.common.subsystem.sftp.SftpConstants.S_IRUSR; +import static org.apache.sshd.common.subsystem.sftp.SftpConstants.S_IWUSR; + import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.File; @@ -38,8 +43,6 @@ import java.util.Vector; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import com.jcraft.jsch.ChannelSftp; -import com.jcraft.jsch.JSch; import org.apache.sshd.client.SshClient; import org.apache.sshd.client.session.ClientSession; import org.apache.sshd.client.subsystem.sftp.extensions.BuiltinSftpClientExtensions; @@ -68,10 +71,8 @@ import org.junit.Ignore; import org.junit.Test; import org.junit.runners.MethodSorters; -import static org.apache.sshd.common.subsystem.sftp.SftpConstants.SSH_FX_FILE_ALREADY_EXISTS; -import static org.apache.sshd.common.subsystem.sftp.SftpConstants.SSH_FX_NO_SUCH_FILE; -import static org.apache.sshd.common.subsystem.sftp.SftpConstants.S_IRUSR; -import static org.apache.sshd.common.subsystem.sftp.SftpConstants.S_IWUSR; +import com.jcraft.jsch.ChannelSftp; +import com.jcraft.jsch.JSch; @FixMethodOrder(MethodSorters.NAME_ASCENDING) public class SftpTest extends AbstractSftpClientTestSupport { @@ -775,7 +776,6 @@ public class SftpTest extends AbstractSftpClientTestSupport { } @Test - @Ignore("Symlinks via Java + SFTP pose some issues") public void testCreateSymbolicLink() throws Exception { // Do not execute on windows as the file system does not support symlinks Assume.assumeTrue("Skip non-Unix O/S", OsUtils.isUNIX()); @@ -784,18 +784,31 @@ public class SftpTest extends AbstractSftpClientTestSupport { Path lclSftp = Utils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(), getCurrentTestName()); Utils.deleteRecursive(lclSftp); + /* + * NOTE !!! according to Jsch documentation + * (see http://epaul.github.io/jsch-documentation/simple.javadoc/com/jcraft/jsch/ChannelSftp.html#current-directory) + * + * + * This sftp client has the concept of a current local directory and + * a current remote directory. These are not inherent to the protocol, + * but are used implicitly for all path-based commands sent to the server + * for the remote directory) or accessing the local file system (for the local directory). + * + * Therefore we are using "absolute" remote files for this test + */ Path parentPath = targetPath.getParent(); Path sourcePath = assertHierarchyTargetFolderExists(lclSftp).resolve("src.txt"); - String remSrcPath = Utils.resolveRelativeRemotePath(parentPath, sourcePath); + String remSrcPath = "/" + Utils.resolveRelativeRemotePath(parentPath, sourcePath); Path linkPath = lclSftp.resolve("link-" + sourcePath.getFileName()); - String remLinkPath = Utils.resolveRelativeRemotePath(parentPath, linkPath); + String remLinkPath = "/" + Utils.resolveRelativeRemotePath(parentPath, linkPath); String data = getCurrentTestName(); ChannelSftp c = (ChannelSftp) session.openChannel(SftpConstants.SFTP_SUBSYSTEM_NAME); c.connect(); try { - c.put(new ByteArrayInputStream(data.getBytes(StandardCharsets.UTF_8)), remSrcPath); - + try (InputStream dataStream = new ByteArrayInputStream(data.getBytes(StandardCharsets.UTF_8))) { + c.put(dataStream, remSrcPath); + } assertTrue("Source file not created: " + sourcePath, Files.exists(sourcePath)); assertEquals("Mismatched stored data in " + remSrcPath, data, readFile(remSrcPath));
