From 4774721b49ab8d80bab598a74b2c02d4e313af5e Mon Sep 17 00:00:00 2001 From: Florian Klemenz Date: Mon, 23 Oct 2023 10:46:14 +0200 Subject: [PATCH] =?UTF-8?q?adds=20fallback=20to=20posix-rename@openssh.com?= =?UTF-8?q?=20extension=20if=20possible=20and=20c=E2=80=A6=20(#827)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * adds fallback to posix-rename@openssh.com 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 Co-authored-by: Jeroen van Erp --- .../net/schmizz/sshj/sftp/SFTPEngine.java | 93 ++++++- .../sshj/sftp/RemoteFileRenameTest.java | 237 ++++++++++++++++++ 2 files changed, 321 insertions(+), 9 deletions(-) create mode 100644 src/test/java/net/schmizz/sshj/sftp/RemoteFileRenameTest.java diff --git a/src/main/java/net/schmizz/sshj/sftp/SFTPEngine.java b/src/main/java/net/schmizz/sshj/sftp/SFTPEngine.java index 9114af35..ecac5afc 100644 --- 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(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(PacketType.INIT).putUInt32(requestedVersion)); final SFTPPacket 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 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. + + // "posix-rename@openssh.com" 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 = "posix-rename@openssh.com"; } + + // 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 \"posix-rename@openssh.com\" 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 new file mode 100644 index 00000000..8f3e6a44 --- /dev/null +++ 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 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 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 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 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 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 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; + } +}