Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File is getting corrupted when Chunk Mode file transfer interrupted #315

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

File is getting corrupted when Chunk Mode file transfer interrupted #315

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c13d8e
ChannelSftp
anamikagsingh Apr 26, 2023
f55eaae
SessionAndUserAuthKeyboardInteractive
anamikagsingh May 2, 2023
9aee720
Support for Multiple Fingerprints And Chunk transfer resumption durin…
anamikagsingh May 15, 2023
8d05634
Chunk transfer resumption during network failure
anamikagsingh May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/main/java/com/jcraft/jsch/ChannelSftp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
import java.util.Hashtable;
import java.util.Vector;

import com.jcraft.jsch.Channel.MyPipedInputStream;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary.
Since ChannelSftp extends Channel, it can directly reference the MyPipedInputStream class without an import statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the import as it was not needed. Thanks for the suggestions.



public class ChannelSftp extends ChannelSession {

static private final int LOCAL_MAXIMUM_PACKET_SIZE = 32 * 1024;
Expand Down Expand Up @@ -158,6 +161,7 @@ public class ChannelSftp extends ChannelSession {
private boolean fEncoding_is_utf8 = true;

private RequestQueue rq = new RequestQueue(16);
private String chunkStart = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does chunkStart need to be a String instead of having it simply be a long?
This would avoid a lot of unwieldy calls to Long.parseLong(chunkStart)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to Long now and made the other changes accordingly.


/**
* Specify how many requests may be sent at any one time. Increasing this value may slightly
Expand Down Expand Up @@ -516,7 +520,13 @@ public void _put(InputStream src, String dst, SftpProgressMonitor monitor, int m
// System.err.println(eee);
}
}
if (mode == RESUME && skip > 0) {
if((mode==RESUME&& chunkStart!=null) && skip>0){
long skipped=src.skip(skip-Long.parseLong(chunkStart));
if(skipped<(skip-Long.parseLong(chunkStart))){
throw new SftpException(SSH_FX_FAILURE, "failed to resume for "+dst);
}
}
else if(mode==RESUME && skip>0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be better written as simply something like this to avoid code duplication:

      if (mode == RESUME && skip > 0) {
        long srcSkip = chunkStart != null ? skip - Long.parseLong(chunkStart) : skip;
        long skipped = src.skip(srcSkip);
        if (skipped < srcSkip) {
          throw new SftpException(SSH_FX_FAILURE, "failed to resume for " + dst);
        }
      }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the changes as suggested.

long skipped = src.skip(skip);
if (skipped < skip) {
throw new SftpException(SSH_FX_FAILURE, "failed to resume for " + dst);
Expand Down Expand Up @@ -643,6 +653,32 @@ public void _put(InputStream src, String dst, SftpProgressMonitor monitor, int m
throw (SftpException) e;
throw new SftpException(SSH_FX_FAILURE, e.toString(), e);
}

public boolean checkChunkFailed(InputStream src, String dst, String ChunkStart)throws SftpException{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The argument InputStream src appears to be unused in this method, so why is it needed?
  2. The argument String ChunkStart should be changed to not start with a capital letter.
    -- Perhaps something like String chunkStart instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Removed the argument InputStream src from the method.
  2. Changed the parameter name to startChunk

try{
((MyPipedInputStream)io_in).updateReadSide();

byte[] dstb=Util.str2byte(dst, fEncoding);
long skip=0;
try{
SftpATTRS attr=_stat(dstb);
skip=attr.getSize();
}
catch(Exception eee){
//System.err.println(eee);
}

if(skip>0){
if(skip == Long.parseLong(ChunkStart))
return false;
else return true;
}
else return false;
}
catch(Exception e){
throw new SftpException(SSH_FX_FAILURE, "Cannot check-" + e.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We should make sure e isn't already an instance of SftpException first, and if it is, just rethrow it directly.
  2. We should also add the original exception as the cause of the new SftpException.
if (e instanceof SftpException)
  throw (SftpException) e;
throw new SftpException(SSH_FX_FAILURE, e.toString(), e);

}

}

public OutputStream put(String dst) throws SftpException {
Expand Down Expand Up @@ -2952,6 +2988,10 @@ public String realpath(String path) throws SftpException {
throw (SftpException) e;
throw new SftpException(SSH_FX_FAILURE, e.toString(), e);
}
}

public void setChunkStart(String chunkStart) {
this.chunkStart= chunkStart;
}

public static class LsEntry implements Comparable<LsEntry> {
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/com/jcraft/jsch/Session.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ public class Session {

Buffer buf;
Packet packet;

List<String> fingerprintList = new ArrayList<String>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be shortened to simply new ArrayList<>() instead of new ArrayList<String>().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it as suggested.


SocketFactory socket_factory = null;

Expand Down Expand Up @@ -883,9 +885,20 @@ private void checkHost(String chost, int port, KeyExchange kex) throws JSchExcep
}
// System.err.println("finger-print: "+key_fprint);
if (userinfo != null) {
if (shkc.equals("ask")) {
boolean foo = userinfo.promptYesNo("The authenticity of host '" + chost
+ "' can't be established.\n" + key_type + " key fingerprint is " + key_fprint + ".\n"
+ "Are you sure you want to continue connecting?");
} else {
anamikagsingh marked this conversation as resolved.
Show resolved Hide resolved
boolean foo = false;
for(String eachFingerPrint : fingerprintList){
if(key_fprint.equalsIgnoreCase(eachFingerPrint))
{
foo = true;
break;
}
}
}
anamikagsingh marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you point at any other applications or libraries that perform SSH host key verification using fingerprints?
This seems like a pretty unusual (and potentially insecure) practice that I am unaware of, and I'm not sure we want to directly enable this sort of thing natively within JSch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if any other application which is using Jsch library is also using fingerprints to perform SSH host key verification. It is being used in SAP PI SFTP Adapter.

if (!foo) {
throw new JSchException("reject HostKey: " + chost);
}
Expand Down Expand Up @@ -3198,6 +3211,12 @@ private void checkConfig(ConfigRepository.Config config, String key) {
if (value != null)
this.setConfig(key, value);
}

public void addToFingerprintList(String channelFingerprint)
{
String []fingerprints = channelFingerprint.split("~");
anamikagsingh marked this conversation as resolved.
Show resolved Hide resolved
this.fingerprintList = new ArrayList<String>(Arrays.asList(fingerprints));
}

/**
* Returns the logger being used by this instance of Session. If no particular logger has been
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/jcraft/jsch/UserAuthKeyboardInteractive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ public boolean start(Session session) throws Exception {
byte[][] response = null;

if (password != null && prompt.length == 1 && !echo[0]
&& prompt[0].toLowerCase().indexOf("password:") >= 0) {
&& (prompt[0].toLowerCase().indexOf("password:") >= 0
|| prompt[0].toLowerCase().indexOf("password") >= 0)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #302 you mention that this change is needed in order to support a server that has a prompt of Enter password:.
However, the string Enter password: already should match prompt[0].toLowerCase().indexOf("password:") >= 0, so I'm not sure I understand the purpose of this change (see below)?

jshell> String foo = "Enter password:";
foo ==> "Enter password:"

jshell> if (foo.toLowerCase().indexOf("password:") >= 0) System.out.println("matches"); else throw new Exception("fail");
matches

jshell>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not able to reproduce this issue in my test system so cannot test it. We can leave the changes done in UserAuthKeyboardInteractive.java for now. Please let me know if I need to revert the changes and commit again. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are not able to reproduce the issue that you believed was fixed by this change, then it should be removed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The or also overlaps, you could just remove the colon..

response = new byte[1][];
response[0] = password;
password = null;
Expand Down