adds fallback to [email protected] extension if possible and c… (

#827) * adds fallback to [email protected] extension if possible and communicates possible problems with flags to the developer * Adds '{}' around if/else statements * adds basic tests for file rename * fix comments * fixes indentation * adds helper methods to make existing sftp rename tests more concise * adds basic test for atomic rewrite * adds possibility to request a specific client version (e.g. for testing purposes) * adds testcases for SFTP rename flags fallback behaviour * refactoring to make SFTPEngine.init(int requestedVersion) protected --------- Co-authored-by: Florian Klemenz <[email protected]> Co-authored-by: Jeroen van Erp <[email protected]>
hierynomus · Oct 23, 2023 · 4774721 · 4774721
1 parent 542bb35
commit 4774721
Show file tree

Hide file tree

Showing 2 changed files with 321 additions and 9 deletions.
diff --git a/src/main/java/net/schmizz/sshj/sftp/SFTPEngine.java b/src/main/java/net/schmizz/sshj/sftp/SFTPEngine.java
@@ -81,7 +81,23 @@ public String canonicalize(String path)
 
     public SFTPEngine init()
             throws IOException {
-        transmit(new SFTPPacket<Request>(PacketType.INIT).putUInt32(MAX_SUPPORTED_VERSION));
+        return init(MAX_SUPPORTED_VERSION);
+    }
+
+    /**
+     * Introduced for internal use by testcases.
+     * @param requestedVersion
+     * @throws IOException
+     */
+    protected SFTPEngine init(int requestedVersion)
+            throws IOException {
+        if (requestedVersion > MAX_SUPPORTED_VERSION)
+            throw new SFTPException("You requested an unsupported protocol version: " + requestedVersion + " (requested) > " + MAX_SUPPORTED_VERSION + " (supported)");
+
+        if (requestedVersion < MAX_SUPPORTED_VERSION)
+            log.debug("Client version {} is smaller than MAX_SUPPORTED_VERSION {}", requestedVersion, MAX_SUPPORTED_VERSION);
+
+        transmit(new SFTPPacket<Request>(PacketType.INIT).putUInt32(requestedVersion));
 
         final SFTPPacket<Response> response = reader.readPacket();
 
@@ -91,7 +107,7 @@ public SFTPEngine init()
 
         operativeVersion = response.readUInt32AsInt();
         log.debug("Server version {}", operativeVersion);
-        if (MAX_SUPPORTED_VERSION < operativeVersion)
+        if (requestedVersion < operativeVersion)
             throw new SFTPException("Server reported incompatible protocol version: " + operativeVersion);
 
         while (response.available() > 0)
@@ -234,16 +250,75 @@ public FileAttributes lstat(String path)
 
     public void rename(String oldPath, String newPath, Set<RenameFlags> flags)
             throws IOException {
-        if (operativeVersion < 1)
+        if (operativeVersion < 1) {
             throw new SFTPException("RENAME is not supported in SFTPv" + operativeVersion);
+        }
 
-        final Request request = newRequest(PacketType.RENAME).putString(oldPath, sub.getRemoteCharset()).putString(newPath, sub.getRemoteCharset());
-        // SFTP Version 5 introduced rename flags according to Section 6.5 of the specification
-        if (operativeVersion >= 5) {
-            long renameFlagMask = 0L;
-            for (RenameFlags flag : flags) {
-                renameFlagMask = renameFlagMask | flag.longValue();
+        // request variables to be determined
+        PacketType type = PacketType.RENAME; // Default
+        long renameFlagMask = 0L;
+        String serverExtension = null;
+
+        if (!flags.isEmpty()) {
+            // SFTP Version 5 introduced rename flags according to Section 6.5 of the specification
+            if (operativeVersion >= 5) {
+                for (RenameFlags flag : flags) {
+                    renameFlagMask = renameFlagMask | flag.longValue();
+                }
+            }
+            // Try to find a fallback solution if flags are not supported by the server.
+
+            // "[email protected]" provides ATOMIC and OVERWRITE behaviour.
+            //  From the SFTP-spec, Section 6.5:
+            // "If SSH_FXP_RENAME_OVERWRITE is specified, the server MAY perform an atomic rename even if it is
+            // not requested."
+            // So, if overwrite is allowed we can always use the posix-rename as a fallback.
+            else if (flags.contains(RenameFlags.OVERWRITE) &&
+                    supportsServerExtension("posix-rename","openssh.com")) {
+
+                type = PacketType.EXTENDED;
+                serverExtension = "[email protected]";
             }
+
+            // Because the OVERWRITE flag changes the behaviour in a possibly unintended way, it has to be
+            // explicitly requested for the above fallback to be applicable.
+            // Tell this to the developer if ATOMIC is requested without OVERWRITE.
+            else if (flags.contains(RenameFlags.ATOMIC) &&
+                    !flags.contains(RenameFlags.OVERWRITE) &&
+                    !flags.contains(RenameFlags.NATIVE) && // see next case below
+                    supportsServerExtension("posix-rename","openssh.com")) {
+                throw new SFTPException("RENAME-FLAGS are not supported in SFTPv" + operativeVersion + " but " +
+                            "the \"[email protected]\" extension could be used as fallback if OVERWRITE " +
+                            "behaviour is acceptable (needs to be activated via RenameFlags.OVERWRITE).");
+            }
+
+            // From the SFTP-spec, Section 6.5:
+            // "If flags includes SSH_FXP_RENAME_NATIVE, the server is free to do the rename operation in whatever
+            // fashion it deems appropriate. Other flag values are considered hints as to desired behavior, but not
+            // requirements."
+            else if (flags.contains(RenameFlags.NATIVE)) {
+                log.debug("Flags are not supported but NATIVE-flag allows to ignore other requested flags: " +
+                        flags.toString());
+            }
+
+            // finally: let the user know that the server does not support what was asked
+            else {
+                throw new SFTPException("RENAME-FLAGS are not supported in SFTPv" + operativeVersion + " and no " +
+                        "supported server extension could be found to achieve a similar result.");
+            }
+        }
+
+        // build and send request
+        final Request request = newRequest(type);
+
+        if (serverExtension != null) {
+            request.putString(serverExtension);
+        }
+
+        request.putString(oldPath, sub.getRemoteCharset())
+                .putString(newPath, sub.getRemoteCharset());
+
+        if (renameFlagMask != 0L) {
             request.putUInt32(renameFlagMask);
         }
 

diff --git a/src/test/java/net/schmizz/sshj/sftp/RemoteFileRenameTest.java b/src/test/java/net/schmizz/sshj/sftp/RemoteFileRenameTest.java
@@ -0,0 +1,237 @@
+/*
+ * Copyright (C)2009 - SSHJ Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package net.schmizz.sshj.sftp;
+
+import com.hierynomus.sshj.test.SshServerExtension;
+import net.schmizz.sshj.SSHClient;
+import net.schmizz.sshj.common.ByteArrayUtils;
+import org.apache.sshd.common.util.io.IoUtils;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.*;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.Random;
+import java.util.Set;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Testing of remote file rename using different combinations of net.schmizz.sshj.sftp.RenameFlags with
+ * possible workarounds for SFTP protocol versions lower than 5 that do not natively support these flags.
+ */
+public class RemoteFileRenameTest {
+    @RegisterExtension
+    public SshServerExtension fixture = new SshServerExtension();
+
+    @TempDir
+    public File temp;
+
+    @Test
+    public void shouldOverwriteFileWhenRequested() throws IOException {
+        // create source file
+        final byte[] sourceBytes = generateBytes(32);
+        File sourceFile = newTempFile("shouldOverwriteFileWhenRequested-source.bin", sourceBytes);
+
+        // create target file
+        final byte[] targetBytes = generateBytes(32);
+        File targetFile = newTempFile("shouldOverwriteFileWhenRequested-target.bin", targetBytes);
+
+        // rename with overwrite
+        Set<RenameFlags> flags = EnumSet.of(RenameFlags.OVERWRITE);
+        SFTPEngine sftp = sftpInit();
+        sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+
+        // check if rename was successful
+        assertThat("The source file should not exist anymore", !sourceFile.exists());
+        assertThat("The contents of the target file should be equal to the contents previously written " +
+                        "to the source file", fileContentEquals(targetFile, sourceBytes));
+    }
+
+    @Test
+    public void shouldNotOverwriteFileWhenNotRequested() throws IOException {
+        // create source file
+        final byte[] sourceBytes = generateBytes(32);
+        File sourceFile = newTempFile("shouldNotOverwriteFileWhenNotRequested-source.bin", sourceBytes);
+
+        // create target file
+        final byte[] targetBytes = generateBytes(32);
+        File targetFile = newTempFile("shouldNotOverwriteFileWhenNotRequested-target.bin", targetBytes);
+
+        // rename without overwrite -> should fail
+        Boolean exceptionThrown = false;
+        Set<RenameFlags> flags = new HashSet<>();
+        SFTPEngine sftp = sftpInit();
+        try {
+            sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+        }
+        catch (SFTPException e) {
+            exceptionThrown = true;
+        }
+
+        // check if rename failed as it should
+        assertThat("The source file should still exist", sourceFile.exists());
+        assertThat("The contents of the target file should be equal to the contents previously written to it",
+                fileContentEquals(targetFile, targetBytes));
+        assertThat("An appropriate exception should have been thrown", exceptionThrown);
+    }
+
+    @Test
+    public void shouldUseAtomicRenameWhenRequestedWithOverwriteOnInsufficientProtocolVersion() throws IOException {
+        // create source file
+        final byte[] sourceBytes = generateBytes(32);
+        File sourceFile = newTempFile("shouldUseAtomicRenameWhenRequestedWithOverwriteOnInsufficientProtocolVersion-source.bin", sourceBytes);
+
+        // create target file
+        final byte[] targetBytes = generateBytes(32);
+        File targetFile = newTempFile("shouldUseAtomicRenameWhenRequestedWithOverwriteOnInsufficientProtocolVersion-target.bin", targetBytes);
+
+        // atomic rename with overwrite -> should work
+        Set<RenameFlags> flags = EnumSet.of(RenameFlags.OVERWRITE, RenameFlags.ATOMIC);
+        int version = Math.min(SFTPEngine.MAX_SUPPORTED_VERSION, 4); // choose a supported version smaller than 5
+        SFTPEngine sftp = sftpInit(version);
+        sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+
+        assertThat("The connection should use the requested protocol version", sftp.getOperativeProtocolVersion() == version);
+        assertThat("The source file should not exist anymore", !sourceFile.exists());
+        assertThat("The contents of the target file should be equal to the contents previously written " +
+                        "to the source file", fileContentEquals(targetFile, sourceBytes));
+    }
+
+
+    @Test
+    public void shouldIgnoreAtomicFlagWhenRequestedWithNativeOnInsufficientProtocolVersion() throws IOException {
+        // create source file
+        final byte[] sourceBytes = generateBytes(32);
+        File sourceFile = newTempFile("shouldIgnoreAtomicFlagWhenRequestedWithNativeOnInsufficientProtocolVersion-source.bin", sourceBytes);
+
+        // create target file
+        final byte[] targetBytes = generateBytes(32);
+        File targetFile = newTempFile("shouldIgnoreAtomicFlagWhenRequestedWithNativeOnInsufficientProtocolVersion-target.bin", targetBytes);
+
+        // atomic flag should be ignored with native
+        // -> should fail because target exists and overwrite behaviour is not requested
+        Boolean exceptionThrown = false;
+        Set<RenameFlags> flags = EnumSet.of(RenameFlags.NATIVE, RenameFlags.ATOMIC);
+        int version = Math.min(SFTPEngine.MAX_SUPPORTED_VERSION, 4); // choose a supported version smaller than 5
+        SFTPEngine sftp = sftpInit(version);
+        try {
+            sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+        }
+        catch (SFTPException e) {
+            exceptionThrown = true;
+        }
+
+        assertThat("The connection should use the requested protocol version", sftp.getOperativeProtocolVersion() == version);
+        assertThat("The source file should still exist", sourceFile.exists());
+        assertThat("The contents of the target file should be equal to the contents previously written to it",
+                fileContentEquals(targetFile, targetBytes));
+        assertThat("An appropriate exception should have been thrown", exceptionThrown);
+
+    }
+
+
+    @Test
+    public void shouldFailAtomicRenameWithoutOverwriteOnInsufficientProtocolVersion() throws IOException {
+        // create source file
+        final byte[] sourceBytes = generateBytes(32);
+        File sourceFile = newTempFile("shouldFailAtomicRenameWithoutOverwriteOnInsufficientProtocolVersion-source.bin", sourceBytes);
+
+        // create target file
+        File targetFile = new File(temp, "shouldFailAtomicRenameWithoutOverwriteOnInsufficientProtocolVersion-target.bin");
+
+        // atomic rename without overwrite -> should fail
+        Boolean exceptionThrown = false;
+        Set<RenameFlags> flags = EnumSet.of(RenameFlags.ATOMIC);
+        int version = Math.min(SFTPEngine.MAX_SUPPORTED_VERSION, 4); // choose a supported version smaller than 5
+        SFTPEngine sftp = sftpInit(version);
+        try {
+            sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+        }
+        catch (SFTPException e) {
+            exceptionThrown = true;
+        }
+
+        // check if rename failed as it should (for version < 5)
+        assertThat("The connection should use the requested protocol version", sftp.getOperativeProtocolVersion() == version);
+        assertThat("The source file should still exist", sourceFile.exists());
+        assertThat("The target file should not exist", !targetFile.exists());
+        assertThat("An appropriate exception should have been thrown", exceptionThrown);
+    }
+
+    @Test
+    public void shouldDoAtomicRenameOnSufficientProtocolVersion() throws IOException {
+        // This test will be relevant as soon as sshj supports SFTP protocol version >= 5
+        if (SFTPEngine.MAX_SUPPORTED_VERSION >= 5) {
+            // create source file
+            final byte[] sourceBytes = generateBytes(32);
+            File sourceFile = newTempFile("shouldDoAtomicRenameOnSufficientProtocolVersion-source.bin", sourceBytes);
+
+            // create target file
+            File targetFile = new File(temp, "shouldDoAtomicRenameOnSufficientProtocolVersion-target.bin");
+
+            // atomic rename without overwrite -> should work on version >= 5
+            Set<RenameFlags> flags = EnumSet.of(RenameFlags.ATOMIC);
+            SFTPEngine sftp = sftpInit();
+            sftp.rename(sourceFile.getPath(), targetFile.getPath(), flags);
+
+            // check if rename worked as it should (for version >= 5)
+            assertThat("The connection should use the requested protocol version", sftp.getOperativeProtocolVersion() >= 5);
+            assertThat("The source file should not exist anymore", !sourceFile.exists());
+            assertThat("The target file should exist", targetFile.exists());
+        }
+        else {
+            // Ignored - cannot test because client does not support protocol version >= 5
+        }
+    }
+
+    private byte[] generateBytes(Integer size) {
+        byte[] randomBytes = new byte[size];
+        Random rnd = new Random();
+        rnd.nextBytes(randomBytes);
+        return randomBytes;
+    }
+
+    private File newTempFile(String name, byte[] content) throws IOException {
+        File tmpFile = new File(temp, name);
+        try (OutputStream fStream = new FileOutputStream(tmpFile)) {
+            IoUtils.copy(new ByteArrayInputStream(content), fStream);
+        }
+        return tmpFile;
+    }
+
+    private boolean fileContentEquals(File testFile, byte[] testBytes) throws IOException {
+        return ByteArrayUtils.equals(
+                IoUtils.toByteArray(new FileInputStream(testFile)), 0,
+                testBytes, 0,
+                testBytes.length);
+    }
+
+    private SFTPEngine sftpInit() throws IOException {
+        return sftpInit(SFTPEngine.MAX_SUPPORTED_VERSION);
+    }
+
+    private SFTPEngine sftpInit(int version) throws IOException {
+        SSHClient ssh = fixture.setupConnectedDefaultClient();
+        ssh.authPassword("test", "test");
+        SFTPEngine sftp = new SFTPEngine(ssh).init(version);
+        return sftp;
+    }
+}